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 AttributeKeyValue abstraction to common otlp exporters #7026

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

jhalliday
Copy link
Contributor

Allows individual Attributes entries (Key/Value tuples) to be represented separately from an Attributes collection and marshalled to OTLP from e.g. List rather than Attributes (i.e. Map).

@jhalliday jhalliday requested a review from a team as a code owner January 16, 2025 11:34
@jhalliday
Copy link
Contributor Author

Initial use case for this more flexible behaviour is profiles requirement to pack N Attributes maps from Java to one repeated opentelemetry.proto.common.v1.KeyValue in protobuf, with KV tuples that appear in multiple maps being packed only once to reduce message size. The tuple abstraction code could live in profiles module for now, but would require changes to the common KeyValueMarshaler to expose non-public utility code, or duplication of it.

Alternatively the existing KeyValue could be used, but would require more conversion (i.e. object creation) and entail type loss through generic type erasure on empty collection values, as KeyValue holds type info on the value, whilst AttributeKeyValue follows Attributes in holding it on the key.

@jhalliday jhalliday changed the title Add AttributeKeyValue abstraction to common API Add AttributeKeyValue abstraction to common otlp exporters Jan 16, 2025
@jack-berg
Copy link
Member

Key bits of context for me that we talked about in slack:

Jack Berg: Is this topic related to the question of whether ProfileData and related *Data types return List<AttributeKeyValue> or Attributes from getAttributesTable()? Looking at the data classes now, we have things like Attributes getAttributesTable(), which is supposed to be referenced by index in other places. But this isn’t possible because there’s no Attributes.get(int index) operation.
And so maybe what you’re talking about here is adjusting the API to be something like List<AttributeKeyValue> getAttributesTable(), which would allow other profile data interfaces to reference the table by index.

Jonathan Halliday: yeah, I changed those types in my local branch, but split the PR so I'm not putting experimental profiling changes in with stable common changes. I can roll it up if you want the change bundled in context.

Jack Berg: Right ok that helps complete the picture. Long term, we anticipate the ProfileData classes being split out into openelemetry-sdk-profile, the profile marshaling code to move to opentelemetry-exporter-otlp-common, and the profile exporter code to move to opentelemetry-exporter-otlp. At this point, both opentelemetry-sdk-otlp-common and opentelemetry-exporter-otlp will have a dependency on opentelemetry-sdk-profile. And so opentelemetry-sdk-profile would be a reasonable place to put AttributeKeyValue.
If it turns out we think that AttributeKeyValue is likely to be used in other SDK contexts, we can move it to opentelemetry-sdk-common.
If it turns out we think that AttributeKeyValue is likely to be used in API contexts, we can move it to opentelemetry-api.

So this PR establishes the AttributeKeyValue abstraction which will be used in a followup PR to adjust profile *Data to use attribute lookup tables referencing List<AttributeKeyValue> instead of Attributes, mirroring the proto message structure.

I dug up a conversation from 3/2024 where we talked about this exact thing: #6374 (comment)

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.

See comment here on context for this PR, since it may not be entirely clear why this is necessary by looking at the code.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 42.10526% with 11 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (c8da020) to head (5df2376).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...etry/exporter/internal/otlp/KeyValueMarshaler.java 0.00% 11 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7026      +/-   ##
============================================
- Coverage     89.95%   89.90%   -0.05%     
- Complexity     6636     6641       +5     
============================================
  Files           745      747       +2     
  Lines         20010    20029      +19     
  Branches       1962     1962              
============================================
+ Hits          17999    18007       +8     
- Misses         1415     1426      +11     
  Partials        596      596              

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

@jack-berg jack-berg merged commit 2fb3eba into open-telemetry:main Jan 16, 2025
24 of 25 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