-
Notifications
You must be signed in to change notification settings - Fork 961
Extended attributes support for log4j-appender-2.17 #13836
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.
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
base: main
Are you sure you want to change the base?
Conversation
ExtendedAttributesBuilder nestedAttribute = ExtendedAttributes.builder(); | ||
Map<?, ?> nestedMap = (Map<?, ?>) value; | ||
consumeAttributes( | ||
consumer -> nestedMap.forEach((k, v) -> consumer.accept(k.toString(), v)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I convert keys of nested maps to string via toString
. What should we do if the key is not a string? Is this fine, or should we throw an exception?
Waiting for #13834 to fix all build errors |
contextData.put("key1", "value1"); | ||
contextData.put("key2", "value2"); | ||
AttributesBuilder attributes = Attributes.builder(); | ||
ExtendedAttributesBuilder attributes = ExtendedAttributes.builder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you elaborate why you didn't just continue using AttributesBuilder
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's only because captureContextDataAttributes
now takes ExtendedAttributesBuilder
instead of AttributesBuilder
. The idea is that I always use ExtendedAttributes
in all methods of LogEventMapper
, and convert to Attributes
at the end if needed.
I figured I could do this because this does not seem to be public API, right?
attributes.put(getMapMessageAttributeKey(key), value.toString()); | ||
} | ||
}); | ||
consumeAttributes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously all values were Strings now they are not. Isn't this a backwards incompatible change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be hidden behind a feature flag? I don't see how we could provide both old and new behavior otherwise
attributes.put( | ||
(ExtendedAttributeKey<Boolean>) keyProvider.apply(key, ExtendedAttributeType.BOOLEAN), | ||
(Boolean) value); | ||
} else if ((value instanceof Long)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elsewhere where we do something similar we also handle Byte and Short. You could use value instanceof Byte || value instanceof Integer || value instanceof Long || value instanceof Short
and ((Number) value).longValue()
to reduce the number of if
statements. The same strategy could also be applied to float and double.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
Object first = list.get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect something horrible will happen when list contains elements of mixed types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I made it fallback to a list of strings: 4f45fc4
Fixes #8354