Extended attributes support for log4j-appender-2.17 #13836
Conversation
github-actions
bot
added
the
test native
| 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.
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.
could you elaborate why you didn't just continue using AttributesBuilder here?
There was a problem hiding this comment.
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.
previously all values were Strings now they are not. Isn't this a backwards incompatible change?
There was a problem hiding this comment.
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.
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.
|
|
||
| Object first = list.get(0); |
There was a problem hiding this comment.
I suspect something horrible will happen when list contains elements of mixed types.
There was a problem hiding this comment.
Here I made it fallback to a list of strings: 4f45fc4
Fixes #8354