Skip to content

feat: improve diff logging #2544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Oct 10, 2024
Merged

feat: improve diff logging #2544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e469ec1
imporve diff logging
bachmanity1 Oct 2, 2024
bd0ff66
compute diff only when actual doesn't match desired
bachmanity1 Oct 2, 2024
4cea98f
slight improvements
bachmanity1 Oct 2, 2024
dcff1ba
increase context size
bachmanity1 Oct 2, 2024
a45f3da
fix style
bachmanity1 Oct 2, 2024
c1b86b8
calculate diff only if debug is enabled
bachmanity1 Oct 2, 2024
611ecdb
print actual resources when trace is enabled
bachmanity1 Oct 2, 2024
57a0ee2
use java-diff-utils
bachmanity1 Oct 2, 2024
962cea7
add unit tests
bachmanity1 Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions operator-framework-core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
<description>Core framework for implementing Kubernetes operators</description>

<dependencies>
<dependency>
<groupId>io.github.java-diff-utils</groupId>
<artifactId>java-diff-utils</artifactId>
</dependency>
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>kubernetes-client</artifactId>
Expand Down
66 changes: 58 additions & 8 deletions ...dk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
Expand All @@ -25,6 +26,9 @@
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.processing.LoggingUtils;

import com.github.difflib.DiffUtils;
import com.github.difflib.UnifiedDiffUtils;

/**
* Matches the actual state on the server vs the desired state. Based on the managedFields of SSA.
*
Expand Down Expand Up @@ -103,20 +107,66 @@ public boolean matches(R actual, R desired, Context<?> context) {

removeIrrelevantValues(desiredMap);

if (LoggingUtils.isNotSensitiveResource(desired)) {
logDiff(prunedActual, desiredMap, objectMapper);
var matches = prunedActual.equals(desiredMap);

if (!matches && log.isDebugEnabled() && LoggingUtils.isNotSensitiveResource(desired)) {
var diff = getDiff(prunedActual, desiredMap, objectMapper);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff should be calculated only if debug enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff should be calculated only if debug enabled.

That's right, this costs a lot.

log.debug(
"Diff between actual and desired state for resource: {} with name: {} in namespace: {} is: \n{}",
actual.getKind(), actual.getMetadata().getName(), actual.getMetadata().getNamespace(),
diff);
}

return prunedActual.equals(desiredMap);
return matches;
}

private void logDiff(Map<String, Object> prunedActualMap, Map<String, Object> desiredMap,
private String getDiff(Map<String, Object> prunedActualMap, Map<String, Object> desiredMap,
KubernetesSerialization serialization) {
if (log.isDebugEnabled()) {
var actualYaml = serialization.asYaml(prunedActualMap);
var desiredYaml = serialization.asYaml(desiredMap);
log.debug("Pruned actual yaml: \n {} \n desired yaml: \n {} ", actualYaml, desiredYaml);
Copy link
Collaborator

@csviri csviri Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still might want to log the actual resources. What do you think?

Copy link
Contributor Author

@bachmanity1 bachmanity1 Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when should we log actual resources?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it is good that we log the diff, but some might be interested in the actual resources (I mean both "actual" and desired yaml). It might be useful to log that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think?

how about logging actual resource in case when logger is enabled for the trace level? or should we introduce new configuration parameter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trace level is fine by me, we could have there both the resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I enabled printing actual and desired resources when trace is enabled.

var actualYaml = serialization.asYaml(sortMap(prunedActualMap));
var desiredYaml = serialization.asYaml(sortMap(desiredMap));
if (log.isTraceEnabled()) {
log.trace("Pruned actual resource: \n {} \ndesired resource: \n {} ", actualYaml,
desiredYaml);
}

var patch = DiffUtils.diff(actualYaml.lines().toList(), desiredYaml.lines().toList());
List<String> unifiedDiff =
UnifiedDiffUtils.generateUnifiedDiff("", "", actualYaml.lines().toList(), patch, 1);
return String.join("\n", unifiedDiff);
}

@SuppressWarnings("unchecked")
Map<String, Object> sortMap(Map<String, Object> map) {
List<String> sortedKeys = new ArrayList<>(map.keySet());
Collections.sort(sortedKeys);

Map<String, Object> sortedMap = new LinkedHashMap<>();
for (String key : sortedKeys) {
Object value = map.get(key);
if (value instanceof Map) {
sortedMap.put(key, sortMap((Map<String, Object>) value));
} else if (value instanceof List) {
sortedMap.put(key, sortListItems((List<Object>) value));
} else {
sortedMap.put(key, value);
}
}
return sortedMap;
}

@SuppressWarnings("unchecked")
List<Object> sortListItems(List<Object> list) {
List<Object> sortedList = new ArrayList<>();
for (Object item : list) {
if (item instanceof Map) {
sortedList.add(sortMap((Map<String, Object>) item));
} else if (item instanceof List) {
sortedList.add(sortListItems((List<Object>) item));
} else {
sortedList.add(item);
}
}
return sortedList;
}

/**
Expand Down
46 changes: 46 additions & 0 deletions ...perator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package io.javaoperatorsdk.operator.processing.dependent.kubernetes;

import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -121,4 +124,47 @@ private <R> R loadResource(String fileName, Class<R> clazz) {
fileName);
}

@Test
@SuppressWarnings("unchecked")
void sortListItemsTest() {
Map<String, Object> nestedMap1 = new HashMap<>();
nestedMap1.put("z", 26);
nestedMap1.put("y", 25);

Map<String, Object> nestedMap2 = new HashMap<>();
nestedMap2.put("b", 26);
nestedMap2.put("c", 25);
nestedMap2.put("a", 24);

List<Object> unsortedListItems = Arrays.asList(1, nestedMap1, nestedMap2);
List<Object> sortedListItems = matcher.sortListItems(unsortedListItems);

assertThat(sortedListItems).element(0).isEqualTo(1);

Map<String, Object> sortedNestedMap1 = (Map<String, Object>) sortedListItems.get(1);
assertThat(sortedNestedMap1.keySet()).containsExactly("y", "z");

Map<String, Object> sortedNestedMap2 = (Map<String, Object>) sortedListItems.get(2);
assertThat(sortedNestedMap2.keySet()).containsExactly("a", "b", "c");
}

@Test
@SuppressWarnings("unchecked")
void testSortMapWithNestedMap() {
Map<String, Object> nestedMap = new HashMap<>();
nestedMap.put("z", 26);
nestedMap.put("y", 25);

Map<String, Object> unsortedMap = new HashMap<>();
unsortedMap.put("b", nestedMap);
unsortedMap.put("a", 1);
unsortedMap.put("c", 2);

Map<String, Object> sortedMap = matcher.sortMap(unsortedMap);

assertThat(sortedMap.keySet()).containsExactly("a", "b", "c");

Map<String, Object> sortedNestedMap = (Map<String, Object>) sortedMap.get("b");
assertThat(sortedNestedMap.keySet()).containsExactly("y", "z");
}
}
6 changes: 6 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
<caffeine.version>3.1.8</caffeine.version>
<mustache.version>0.9.11</mustache.version>
<commons.io.version>2.16.1</commons.io.version>
<java.diff.version>4.12</java.diff.version>

<fmt-maven-plugin.version>2.11</fmt-maven-plugin.version>
<maven-compiler-plugin.version>3.12.1</maven-compiler-plugin.version>
Expand Down Expand Up @@ -161,6 +162,11 @@
<version>${mokito.version}</version>
</dependency>

<dependency>
<groupId>io.github.java-diff-utils</groupId>
<artifactId>java-diff-utils</artifactId>
<version>${java.diff.version}</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
Expand Down
Loading