Skip to content

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

fandreuz
Copy link

Fixes #8354

@fandreuz fandreuz requested a review from a team as a code owner May 10, 2025 10:59
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label May 10, 2025
ExtendedAttributesBuilder nestedAttribute = ExtendedAttributes.builder();
Map<?, ?> nestedMap = (Map<?, ?>) value;
consumeAttributes(
consumer -> nestedMap.forEach((k, v) -> consumer.accept(k.toString(), v)),
Copy link
Author

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?

@fandreuz
Copy link
Author

Waiting for #13834 to fix all build errors

contextData.put("key1", "value1");
contextData.put("key2", "value2");
AttributesBuilder attributes = Attributes.builder();
ExtendedAttributesBuilder attributes = ExtendedAttributes.builder();
Copy link
Contributor

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?

Copy link
Author

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(
Copy link
Contributor

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?

Copy link
Author

@fandreuz fandreuz May 13, 2025

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)) {
Copy link
Contributor

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.

Copy link
Author

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);
Copy link
Contributor

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.

Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTelemetry Log4j appender does not handle non string values
2 participants