feat: improve diff logging #2544

bachmanity1 · 2024-10-02T07:43:39Z

Based on this comment.

Diff is calculated and printed only if there is a mismatch between actual and desired state and logger is enabled for the debug level. Here is the sample output:

2024-10-02 16:38:42,233 1 i.j.o.p.d.k.SSABasedGenericKubernetesResourceMatcher [DEBUG] Diff between actual and desired state for resource: ConfigMap with name: test1 in namespace: default is: 
--- 
+++ 
@@ -2,3 +2,3 @@
 data:
-  key1: "val1"
+  key1: "different value"
 metadata:

csviri · 2024-10-02T08:04:55Z

...orsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java

+    var matches = prunedActual.equals(desiredMap);
+
+    if (!matches && LoggingUtils.isNotSensitiveResource(desired)) {
+      var diff = getDiff(prunedActual, desiredMap, objectMapper);


The diff should be calculated only if debug enabled.

The diff should be calculated only if debug enabled.

That's right, this costs a lot.

csviri · 2024-10-02T08:05:20Z

...orsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java

-    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);


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

when should we log actual resources?

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?

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?

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

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

Signed-off-by: bachmanity1 <[email protected]>

10000-ki · 2024-10-02T08:49:17Z

operator-framework-core/pom.xml

@@ -58,7 +58,6 @@
    <dependency>
      <groupId>org.assertj</groupId>
      <artifactId>assertj-core</artifactId>
-      <scope>test</scope>


I am not sure if it is right to include assertj as a dependency when build time rather than in a test environment.

Yes, if this could be done nicer, would be great.

possible alternative is to use https://github.com/java-diff-utils/java-diff-utils

Let me know if I should remove the last commit or not.

possible alternative is to use https://github.com/java-diff-utils/java-diff-utils

This looks good to me, the library itself doesn't seem heavy.
It looks like there may be another opportunity to use this library for other purposes in the future.

Yes, I was going to comment on using this lib instead of assertj. Thanks!

Signed-off-by: bachmanity1 <[email protected]>

10000-ki · 2024-10-02T09:55:09Z

...orsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java

+    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")
+  private 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) {
+        Map<String, Object> nestedMap = (Map<String, Object>) value;
+        sortedMap.put(key, sortMap(nestedMap));
+      } else if (value instanceof List) {
+        sortedMap.put(key, sortList((List<?>) value));
+      } else {
+        sortedMap.put(key, value);
+      }
+    }
+    return sortedMap;
+  }
+
+  @SuppressWarnings("unchecked")
+  private List<?> sortList(List<?> list) {
+    List<Object> sortedList = new ArrayList<>();
+    for (Object item : list) {
+      if (item instanceof Map) {
+        Map<String, Object> mapItem = (Map<String, Object>) item;
+        sortedList.add(sortMap(mapItem));
+      } else if (item instanceof List) {
+        sortedList.add(sortList((List<?>) item));
+      } else {
+        sortedList.add(item);
+      }
    }
+    return sortedList;


What if you could create a separate utility class like DiffGenerator and write unit tests for it?

makes sense

I'm sorry, I was planning to separate this into util class but I got a little busy lately.

Np, feel free to create a separate PR. thx

metacosm · 2024-10-02T14:41:16Z

LGTM but would indeed like to see some tests.

csviri · 2024-10-02T14:56:24Z

...orsdk/operator/processing/dependent/kubernetes/SSABasedGenericKubernetesResourceMatcher.java

+  }
+
+  @SuppressWarnings("unchecked")
+  private Map<String, Object> sortMap(Map<String, Object> map) {


Yes, for this pls add unit tests.

csviri

LGTM thx!

metacosm

Thank you!

* imporve diff logging Signed-off-by: bachmanity1 <[email protected]> * compute diff only when actual doesn't match desired Signed-off-by: bachmanity1 <[email protected]> * slight improvements Signed-off-by: bachmanity1 <[email protected]> * increase context size Signed-off-by: bachmanity1 <[email protected]> * fix style Signed-off-by: bachmanity1 <[email protected]> * calculate diff only if debug is enabled Signed-off-by: bachmanity1 <[email protected]> * print actual resources when trace is enabled Signed-off-by: bachmanity1 <[email protected]> * use java-diff-utils Signed-off-by: bachmanity1 <[email protected]> * add unit tests --------- Signed-off-by: bachmanity1 <[email protected]>

openshift-ci bot requested review from adam-sandor and csviri October 2, 2024 07:43

bachmanity1 changed the title ~~Improve diff logging~~ feat: improve diff logging Oct 2, 2024

bachmanity1 force-pushed the improve-diff-logging branch from d1e477b to 03e5c5e Compare October 2, 2024 07:54

csviri requested changes Oct 2, 2024

View reviewed changes

bachmanity1 added 7 commits October 2, 2024 17:44

imporve diff logging

e469ec1

Signed-off-by: bachmanity1 <[email protected]>

compute diff only when actual doesn't match desired

bd0ff66

Signed-off-by: bachmanity1 <[email protected]>

slight improvements

4cea98f

Signed-off-by: bachmanity1 <[email protected]>

increase context size

dcff1ba

Signed-off-by: bachmanity1 <[email protected]>

fix style

a45f3da

Signed-off-by: bachmanity1 <[email protected]>

calculate diff only if debug is enabled

c1b86b8

Signed-off-by: bachmanity1 <[email protected]>

print actual resources when trace is enabled

611ecdb

Signed-off-by: bachmanity1 <[email protected]>

bachmanity1 force-pushed the improve-diff-logging branch from b116919 to 611ecdb Compare October 2, 2024 08:44

10000-ki reviewed Oct 2, 2024

View reviewed changes

use java-diff-utils

57a0ee2

Signed-off-by: bachmanity1 <[email protected]>

10000-ki reviewed Oct 2, 2024

View reviewed changes

csviri reviewed Oct 2, 2024

View reviewed changes

add unit tests

962cea7

csviri approved these changes Oct 10, 2024

View reviewed changes

csviri requested a review from metacosm October 10, 2024 06:37

metacosm approved these changes Oct 10, 2024

View reviewed changes

csviri merged commit 5585765 into operator-framework:next Oct 10, 2024
19 of 20 checks passed

bachmanity1 deleted the improve-diff-logging branch October 10, 2024 12:01

This was referenced Dec 3, 2024

chore: fabri8 client to v7.0.0 #2616

Merged

docs: migration guide as blogpost refactor #2618

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve diff logging #2544

feat: improve diff logging #2544

bachmanity1 commented Oct 2, 2024 •

edited

Loading

csviri Oct 2, 2024

10000-ki Oct 2, 2024

csviri Oct 2, 2024 •

edited

Loading

bachmanity1 Oct 2, 2024 •

edited

Loading

csviri Oct 2, 2024

bachmanity1 Oct 2, 2024

csviri Oct 2, 2024

bachmanity1 Oct 2, 2024

10000-ki Oct 2, 2024

csviri Oct 2, 2024

bachmanity1 Oct 2, 2024

bachmanity1 Oct 2, 2024

10000-ki Oct 2, 2024

metacosm Oct 2, 2024

10000-ki Oct 2, 2024

csviri Oct 2, 2024

bachmanity1 Oct 10, 2024

csviri Oct 10, 2024

metacosm commented Oct 2, 2024

csviri Oct 2, 2024

csviri left a comment

metacosm left a comment

feat: improve diff logging #2544

feat: improve diff logging #2544

Conversation

bachmanity1 commented Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

bachmanity1 Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacosm commented Oct 2, 2024

Choose a reason for hiding this comment

csviri left a comment

Choose a reason for hiding this comment

metacosm left a comment

Choose a reason for hiding this comment

bachmanity1 commented Oct 2, 2024 •

edited

Loading

csviri Oct 2, 2024 •

edited

Loading

bachmanity1 Oct 2, 2024 •

edited

Loading