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

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

Open
lmolkova opened this issue Sep 3, 2024 · 5 comments
Open
Labels

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.

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

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

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

@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 4, 2024

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

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

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants