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.

Sign up for GitHub

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

Jump to bottom

Event API should document whether it supports complex attribute types #4199

Open
lmolkova opened this issue Sep 3, 2024 · 9 comments · May be fixed by #4334
Open

Event API should document whether it supports complex attribute types #4199

lmolkova opened this issue Sep 3, 2024 · 9 comments · May be fixed by #4334
Labels
sig-issue A specific SIG should look into this before discussing at the spec spec:logs Related to the specification/logs directory

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Sep 3, 2024

We restrict attribute types to primitives and array or primitives and make an exception for LogRecord.

opentelemetry-specification/specification/common/README.md

Lines 76 to 80 in e9a2b00

The standard attribute definition SHOULD be used to represent attributes in data
modeling unless there is a strong justification to diverge. For example, the Log
Data Model has an extended [attributes](../logs/data-model.md#field-attributes)
definition allowing values of [type `Any`](../logs/data-model.md#type-any). This
reflects that LogRecord attributes are expected to model data produced from

See #3858 for the context.

Semconv event definition however limits events attributes and does not allow them to have complex types.

It MAY have standard
attributes that provide additional context about the event.

https://github.com/open-telemetry/semantic-conventions/blob/11f5d37d1624c2392ea97f40e76c1b3c7cd30124/docs/general/events.md?plain=1#L61

We should explicitly call out supported attribute types for events in the spec so that the restriction applies to all events and not just those defined in semantic conventions (or decide that events support complex types).
@lmolkova lmolkova added the spec:logs Related to the specification/logs directory label Sep 3, 2024
@MSNev
Copy link
Contributor

MSNev commented Sep 3, 2024

For the "attributes" it should be restricted to "standard" attributes as called out.

For the "body" fields, (payload_ (body)) when fields are provided (as attributes) they should be log attributes

@pellared
Copy link
Member

pellared commented Sep 3, 2024 •
edited
Loading

It should be specified in Event API doc that Emit Event attributes are restricted to standard attributes.

@trask trask added the triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted label Sep 3, 2024
@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 4, 2024 •
edited
Loading

BTW what would be the reason NOT to support complex types for custom (non-semconv) events?

opentelemetry-specification/specification/common/README.md

Lines 84 to 97 in e9a2b00

Note: Extending the set of standard attribute value types is a breaking change.
This was decided after extensive debate, with arguments as follows:
* Limiting the types of attribute values to a set which has proved sufficient
during several years of OpenTelemetry's development is a useful guardrail for
design. In taking additional value types off the table, we narrow the solution
space and have more productive design conversations.
* Upon proposing to extend support for complex value types, we received significant
pushback. Limiting attribute value types to primitives and arrays of primitives
simplifies data consumers' efforts to create search indexes and perform statistical
analysis.
* To address concerns over restricting standard attributes to primitive types, it was
called out that complex types can be encoded as existing primitive types, such as
representing datetime as a string or 64 bit integer.

lists backward-compatibility (not appliable here), indexing, analysis.

I assume that customer reporting the same event using event or logging API should be able to achieve the same outcome.
In other words, we should not ask users to use 3rd party structured logger to emit events with complex attribute types.

And if they chose to report their own events with complex attribute structure, they probably need to take possible querying, indexing, and other problems into consideration.

It does not mean that we should lift restrictions on the semconv side - otel semconv attributes are cross-signal by definition and can't support complex types. Any events emitted by otel instrumentation will use standard attributes.

@MSNev
Copy link
Contributor

MSNev commented Sep 4, 2024

BTW what would be the reason NOT to support complex types for custom (non-semconv) events?

For the "standard" attributes, this was (mostly) because SDK implementations don't support (or correctly support) being passed a complex attribute (even though Protobuf does support) and then the decision was made to restrict the difference between "standard" and "log (extended) attributes).

For the "body" the intent IS to support nested attributes, and I believe in the JS prototype we added support for a LogAttribute (which did not previously exist)

@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 4, 2024

For the "standard" attributes, this was (mostly) because SDK implementations don't support (or correctly support) being passed a complex attribute (even though Protobuf does support) and then the decision was made to restrict the difference between "standard" and "log (extended) attributes).

I don't understand this.

Logs spec explicitly allows to use complex attribute types as an exception.

opentelemetry-specification/specification/common/README.md

Lines 77 to 82 in e9a2b00

modeling unless there is a strong justification to diverge. For example, the Log
Data Model has an extended [attributes](../logs/data-model.md#field-attributes)
definition allowing values of [type `Any`](../logs/data-model.md#type-any). This
reflects that LogRecord attributes are expected to model data produced from
external log APIs, which do not necessarily have the same value type
restrictions as the standard attribute definition.

So if SDKs don't support it it's a bug.

I believe that events should follow the same pattern since they are based on logs.

@cijothomas cijothomas mentioned this issue Sep 5, 2024
Open
@github-actions github-actions bot added the triage:followup Needs follow up during triage label Sep 19, 2024
@svrnm svrnm removed the triage:followup Needs follow up during triage label Sep 30, 2024
@svrnm
Copy link
Member

svrnm commented Sep 30, 2024

this issue has stalled, are there any updates? What is required to move it forward?

@pellared pellared mentioned this issue Sep 30, 2024
Closed
@cijothomas
Copy link
Member

this issue has stalled, are there any updates? What is required to move it forward?

Waiting till #4225 is settled would be best, given there are still open discussions/questions?

@github-actions github-actions bot added the triage:followup Needs follow up during triage label Oct 1, 2024
@mtwo mtwo removed the triage:followup Needs follow up during triage label Oct 1, 2024
@trask trask added sig-issue A specific SIG should look into this before discussing at the spec and removed triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted labels Oct 2, 2024
@trask trask added this to Logs SIG Oct 2, 2024
@pellared pellared mentioned this issue Oct 15, 2024
Merged
@MSNev
Copy link
Contributor

MSNev commented Oct 15, 2024

I believe that this issue should now be closed based on the outcome and this PR which prohibits "standard" attributes from evolving to contain complex types.

We should have all explicit "Attribute" be standard "Attributes" from a Sem-Convention perspective, and ensure that from an OpenTelemetry perspective the "complex" types "should" be restricted to the "body" -- Although I do appreciate that "Logs" and "LogAttributes" at the protocol level can support nesting...

@lmolkova
Copy link
Contributor Author

lmolkova commented Oct 15, 2024 •
edited
Loading

That PR explicitly allowed AnyValue attribute values on LogRecords as an exception and the current version of the spec language is cited in the issue description and comments.

In particular

opentelemetry-specification/specification/common/README.md

Lines 77 to 82 in e9a2b00

modeling unless there is a strong justification to diverge. For example, the Log
Data Model has an extended [attributes](../logs/data-model.md#field-attributes)
definition allowing values of [type `Any`](../logs/data-model.md#type-any). This
reflects that LogRecord attributes are expected to model data produced from
external log APIs, which do not necessarily have the same value type
restrictions as the standard attribute definition.

@lmolkova lmolkova mentioned this issue Dec 10, 2024
Open
@trask trask linked a pull request Dec 10, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
sig-issue A specific SIG should look into this before discussing at the spec spec:logs Related to the specification/logs directory
Projects
Logs SIG
Status: No status
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support complex attributes in the Event API trask/opentelemetry-specification
7 participants
@trask @mtwo @svrnm @lmolkova @pellared @cijothomas @MSNev