Conversation
Fixes #3183 Some (all?) MDC implementations prevent adding null values so default should be provided or the key omitted. Signed-off-by: Chris Laprun <metacosm@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses MDC-related null handling during event/resource logging (Fixes #3183) by centralizing how resource information is written to and removed from MDC, especially for informer event handling where clustered (namespace-less) resources can surface null metadata fields.
Changes:
- Refactors informer event MDC population to reuse
addResourceInfo(...)/removeResourceInfo(...)via a newforEventSourceflag. - Introduces an event-source key prefixing mechanism and a default
"unknown action"value when theactionis null. - Adjusts removal logic to match the new prefixed key strategy.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/MDCUtils.java
Outdated
Show resolved
Hide resolved
| if (enabled) { | ||
| MDC.put(API_VERSION, resource.getApiVersion()); | ||
| MDC.put(KIND, resource.getKind()); | ||
| MDC.put(key(API_VERSION, forEventSource), resource.getApiVersion()); | ||
| MDC.put(key(KIND, forEventSource), resource.getKind()); | ||
| final var metadata = resource.getMetadata(); | ||
| if (metadata != null) { | ||
| MDC.put(NAME, metadata.getName()); | ||
| MDC.put(key(NAME, forEventSource), metadata.getName()); | ||
| if (metadata.getNamespace() != null) { | ||
| MDC.put(NAMESPACE, metadata.getNamespace()); | ||
| MDC.put(key(NAMESPACE, forEventSource), metadata.getNamespace()); | ||
| } | ||
| MDC.put(RESOURCE_VERSION, metadata.getResourceVersion()); | ||
| MDC.put(key(RESOURCE_VERSION, forEventSource), metadata.getResourceVersion()); | ||
| if (metadata.getGeneration() != null) { | ||
| MDC.put(GENERATION, metadata.getGeneration().toString()); | ||
| MDC.put(key(GENERATION, forEventSource), metadata.getGeneration().toString()); | ||
| } | ||
| MDC.put(UID, metadata.getUid()); | ||
| MDC.put(key(UID, forEventSource), metadata.getUid()); |
There was a problem hiding this comment.
This change aims to avoid MDC backends that reject nulls, but MDC.put(...) is still called with values that can be null (e.g., resource.getApiVersion(), resource.getKind(), metadata.getName(), metadata.getResourceVersion(), metadata.getUid()). To fully prevent NPEs/IAEs, guard these puts (e.g., only put when non-null, otherwise omit/remove or supply a default).
There was a problem hiding this comment.
I think that these other values should never be null for "real" resources coming from a live cluster so this should be fine.
| if (enabled) { | ||
| MDC.put(EVENT_RESOURCE_NAME, resource.getMetadata().getName()); | ||
| MDC.put(EVENT_RESOURCE_NAMESPACE, resource.getMetadata().getNamespace()); | ||
| MDC.put(EVENT_RESOURCE_KIND, HasMetadata.getKind(resource.getClass())); | ||
| MDC.put(EVENT_RESOURCE_VERSION, resource.getMetadata().getResourceVersion()); | ||
| MDC.put(EVENT_RESOURCE_UID, resource.getMetadata().getUid()); | ||
| MDC.put(EVENT_ACTION, action == null ? null : action.name()); | ||
| addResourceInfo(resource, true); | ||
| MDC.put(EVENT_ACTION, action == null ? UNKNOWN_ACTION : action.name()); | ||
| MDC.put(EVENT_SOURCE_NAME, eventSourceName); | ||
| } |
There was a problem hiding this comment.
MDC.put(EVENT_SOURCE_NAME, eventSourceName) will still fail on MDC implementations that don't accept null values if eventSourceName is null. Consider applying the same strategy as for the action (default value) or omitting/removing the key when the name is null.
There was a problem hiding this comment.
eventSourceName is never supposed to be null.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/MDCUtils.java
Outdated
Show resolved
Hide resolved
| import io.javaoperatorsdk.operator.processing.event.ResourceID; | ||
| import io.javaoperatorsdk.operator.processing.event.source.ResourceAction; | ||
|
|
||
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
@SuppressWarnings("unused") on the whole utility class is very broad and can hide legitimate unused warnings for newly added members. If this was added to address a specific warning, consider narrowing its scope (or removing it if it's no longer needed).
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
This is a utility class that users are expected to use in their own code so having unused methods is fine, imo. And actually, removing such methods would be a breaking API change because, while they might not be used within the context of the project, they might be used in users' projects.
There was a problem hiding this comment.
This is not released we can remove those IMO, not sure were user would use that.
There was a problem hiding this comment.
You added them so I didn't want to remove them without your agreement since I wasn't sure what their purpose was.
There was a problem hiding this comment.
Yes, sorry, my mistake. Feel free to remove it.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Chris Laprun <metacosm@gmail.com>
|
I'm actually not sure how to test that because to be useful, it would need a whole bunch of infrastructure to add an MDC backend and would need to check log messages, which is quite tedious to do, typically and is also dependent on logging backends, so it definitely wouldn't be a unit test, in any case. |
There is a get in MDC to return the value, but it is not important IMO, since this is trivial logic mostly |
Maybe, I'm not familiar with MDC… The issue, though, is that the |
np, I think this is good as it is now. |
2 participants
Fixes #3183
Some (all?) MDC implementations prevent adding null values so default
should be provided or the key omitted.
Signed-off-by: Chris Laprun metacosm@gmail.com