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

Support complex attributes in the Event API #4334

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trask
Copy link
Member

@trask trask commented Dec 10, 2024

Fixes #4199

Allow complex attributes on Events.

Based on discussion in open-telemetry/semantic-conventions#1651 and in today's Specification SIG meeting (recording).

The biggest struggle currently for launching the Event API is the "two property bags" problem, also known as "do I put my event key/value pairs into event attributes or into the event body?"

The Log SIG has been trying to figure out a satisfactory answer to this for a month.

The proposal from @lmolkova is to put everything into event attributes, eliminating one of these "property bags" and answering the question of where to put event key/value pairs.

The event body could still potentially be used for dumping unstructured (or possibly very large) data.

To make this work, we need complex attributes on Events.

This would allow for example "locally scoped" attributes on an event to be defined as a complex attribute in the semantic conventions, e.g. an event named browser.page_navigation_timing could define a complex attribute in the semantic conventions named browser.page_navigation_timing (could be the same as the event name or different), which would contain it's "locally scoped" event attributes underneath it, e.g.

{
  "user.name": ...,
  ...,
  "browser.page_navigation_timing": {
    "name": ...,
    "fetchStart": ...,
    "unloadEventStart": ...,
    "unloadEventEnd": ...,
    ...
  },
  ...
}

@trask trask force-pushed the complex-event-attributes branch from 19dc563 to 115a189 Compare December 10, 2024 23:56
@trask trask marked this pull request as ready for review December 10, 2024 23:57
@trask trask requested review from a team as code owners December 10, 2024 23:57
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

A prototype in OTel Go: open-telemetry/opentelemetry-go#6017

The Go SIG strongly prefers: open-telemetry/opentelemetry-go#6018. However, it is also using supporting complex log attributes. Therefore, still an approval from my side as it makes this prototype more aligned.

@austinlparker
Copy link
Member

Quick question/clarification - since this strikes me as extending the common attribute definition (https://opentelemetry.io/docs/specs/otel/common/), are we limiting the depth of complex attributes?

@reyang
Copy link
Member

reyang commented Dec 11, 2024

We have limits on the number of attributes https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#readablelogrecord. This PR would give people a workaround if they just add everything to a single complex attribute - which is concerning from reliability/performance/security perspective.

@pellared
Copy link
Member

pellared commented Dec 11, 2024

This PR would give people a workaround if they just add everything to a single complex attribute - which is concerning from reliability/performance/security perspective.

The attribute limiting issue is already present in the existing Logs (Bridge) API. What is more there are no limits for the Body. More:

@trask
Copy link
Member Author

trask commented Dec 11, 2024

We discussed in the Log SIG yesterday and decided that these both need to be addressed before stabilizing Events:

  • Length limits for complex attribute bodies
  • What does "number of attributes" limit mean for logs?

I'll open a tracking issue. EDIT #4335

But we don't think it's a blocker for this PR since it's a pre-existing issue for Log attributes.

@reyang
Copy link
Member

reyang commented Dec 11, 2024

But we don't think it's a blocker for this PR since it's a pre-existing issue for Log attributes.

+1

@@ -185,8 +185,7 @@ formatted as an [event](./data-model.md#events).
* [Severity Number](./data-model.md#field-severitynumber) (optional)
* [Severity Text](./data-model.md#field-severitytext) (optional)
* [Body](./data-model.md#field-body) (optional)
* [Attributes](./data-model.md#field-attributes) (optional) Event `Attributes` conform
to the [standard definition](../common/README.md#standard-attribute) of an attribute.
* [Attributes](./data-model.md#field-attributes) (optional)
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if we can limit this to standard attributes plus "maps" and "arrays of maps"

where map values can be

  • primitives
  • heterogenous array of primitives
  • maps
  • array of maps

in other words, do we need these?

  • non-heterogeneous arrays
  • arrays of arrays

Copy link
Member

@pellared pellared Dec 15, 2024

Choose a reason for hiding this comment

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

I think that such limitation would just add more complexity given that EmitLogRecord needs to support it.

Having different abstractions for log and event attributes seems an overkill.

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.

Event API should document whether it supports complex attribute types
5 participants