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 7 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
1 change: 0 additions & 1 deletion operator-framework-core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

</dependency>
<dependency>
<groupId>io.fabric8</groupId>
Expand Down
65 changes: 57 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 @@ -13,6 +14,7 @@
import java.util.SortedMap;
import java.util.TreeMap;

import org.assertj.core.util.diff.DiffUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -103,20 +105,67 @@ 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 =
DiffUtils.generateUnifiedDiff("", "", actualYaml.lines().toList(), patch, 1);
return String.join("\n", unifiedDiff);
}

@SuppressWarnings("unchecked")
private Map<String, Object> sortMap(Map<String, Object> map) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, for this pls add unit tests.

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

/**
Expand Down
Loading