-
Notifications
You must be signed in to change notification settings - Fork 847
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
feat/logattributes support map type #3821
feat/logattributes support map type #3821
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3821 +/- ##
==========================================
+ Coverage 92.94% 92.96% +0.01%
==========================================
Files 299 299
Lines 9075 9082 +7
Branches 1851 1855 +4
==========================================
+ Hits 8435 8443 +8
+ Misses 640 639 -1
|
ffd5d8a
to
bac7e23
Compare
bac7e23
to
9df8522
Compare
Can you please provide a link to the relevant spec to make sure reviewers have it handy? /cc @martinkuba |
Here is a link for the spec containing event.data as a map of attributes, https://github.com/scheler/opentelemetry-specification/blob/rum-events/specification/logs/events/semantic_conventions/browser.md#pageview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR intended as a prototype for an upcoming spec, if yes, would you mind providing a link to the related PR? 🙂
I looked through the spec, and I could not find any special handling for LogAttributes
that differs from the common attribute definition, which states that:
An
Attribute
is a key-value pair, which MUST have the following properties:
- The attribute key MUST be a non-
null
and non-empty string.- The attribute value is either:
- A primitive type: string, boolean, double precision floating point (IEEE 754-1985) or signed 64 bit integer.
- An array of primitive type values. The array MUST be homogeneous,
i.e., it MUST NOT contain values of different types.
I'm not at all against adding support for maps in event attributes, but I think we should reference the specification (or a PR in the meantime) first to have this documented somwhere.
This is a fork of the spec. The current version of spec has no mention of map type attributes. |
@martinkuba is the @scheler fork considered to be the prototype used by the RUM or logs SIG? |
@dyladan @pichlermarc This is the correct link to the spec: The log data model always supported complex attributes. We want to leverage this support to put Event payload/data into an attribute called |
Ah, that makes sense, thanks for the link. 🙂 Now looking at it again, I see that
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'd like to get @martinkuba 👀 on this as well if we can
@martinkuba wrt the discussion yesterday it looks like he made the change you suggested on the call for arbitrarily nested maps |
Which problem is this PR solving?
Adds support for map in attributes for logs (for events to create nested attributes as in event.data)
Short description of the changes
Make type changes in logs api and respective changes in the logs sdk, otlp-transformer
Type of change
How Has This Been Tested?
Checklist: