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

Add put(AttributeKey<T>, T) overload to EventBuilder #6331

Merged

Conversation

jack-berg
Copy link
Member

Event payload fields seem to be different than attributes: open-telemetry/semantic-conventions#505

That's expected to some degree, since attribute types are limited to primitives and arrays of primitives, and event payloads are intended to be able to encode complex structured data, like bytes, object arrays, and maps of maps. But excluding Attributes / AttributesKey<?> from the event payload API surface area seems odd, given how prevalent Attributes are in the other APIs.

What happens if I want to include an attribute generated from semantic-conventions-java in my payload? Am I prohibited from doing that? Do I need to call put(AttributeKey.getKey(), value) to extract the string attribute key and loose type safety? This is poor ergonomics.

I'm not exactly sure where things will land with how attributes and event fields overlap (if at all), but it seems like we ought to support our notion of AttributeKey in the API regardless. This PR adds that surface area. We might also consider adding a putAll(Attributes) convenience mechanism for adding all the key value from a bundle of attributes into the payload.

@jack-berg jack-berg requested a review from a team March 29, 2024 22:52
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 91.07%. Comparing base (98eded9) to head (e19cabb).

Files Patch % Lines
...entelemetry/api/incubator/events/EventBuilder.java 88.88% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6331      +/-   ##
============================================
- Coverage     91.07%   91.07%   -0.01%     
- Complexity     5751     5759       +8     
============================================
  Files           626      626              
  Lines         16782    16800      +18     
  Branches       1718     1718              
============================================
+ Hits          15285    15300      +15     
- Misses         1003     1004       +1     
- Partials        494      496       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

🚢

@jack-berg jack-berg merged commit 1623a80 into open-telemetry:main Apr 18, 2024
18 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.

2 participants