Skip to content
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

Extended jinja template to generate template-type semantic attributes. #24

Merged
merged 6 commits into from
Nov 21, 2023

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented Sep 21, 2023

With open-telemetry/build-tools#186, template-type attributes (such as http.request.header.<key>) can be defined in semantic convention model yaml files. With that, these template attributes can be used in code generation for the semantic conventions.

This PR adds a new class AttributeKeyTemplate for this kind of attribute keys and extends the code generation to cover attributes like http.request.header.<key>, db.elasticsearch.path_parts.<key>, rpc.grpc.request.metadata.<key>, etc.

With this change utility classes like CapturedHttpHeadersUtil in the java-instrumentation repo can be replaced with instances (and corresponding constants) of the more generic AttributeKeyTemplate class.

@AlexanderWert AlexanderWert marked this pull request as ready for review October 29, 2023 09:00
@AlexanderWert AlexanderWert requested a review from a team October 29, 2023 09:00
Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@AlexanderWert
Copy link
Member Author

@open-telemetry/java-approvers PTAL, I think this would allow to replace some custom helpers in the agent with generated constants for this type of attributes

@trask
Copy link
Member

trask commented Oct 31, 2023

cc @breedx-splk @mateuszrzeszutek to review new public API surface if you have a chance

Copy link

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

This looks good to me. Just have the one idea around the key cache being static, but otherwise seems like a nice addition.

Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@AlexanderWert
Copy link
Member Author

@jack-berg , @trask , @mateuszrzeszutek , @breedx-splk

Based on the discussions above I reverted the implementation to use a final , inline-initialized map (with initial size 1) again.
PTA(nother)L, Thanks!

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

LGTM, I left one more minor JavaDoc comment.
Thanks!

Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@AlexanderWert
Copy link
Member Author

OK, then I think it's ready to be merged.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Couple nits and a question about whether the methods to create AttributeKeyTemplate should be public to allow users to create their own templates. None of the comments should block this PR.

@jack-berg jack-berg mentioned this pull request Nov 17, 2023
@jack-berg jack-berg merged commit f78c32b into open-telemetry:main Nov 21, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants