Skip to content

Conversation

@lmolkova
Copy link
Member

@lmolkova lmolkova commented Mar 27, 2025

Changes

Document that Extended Attribute term should be used to represent an extended set of standard attributes (if introduced by SDK).
This leaves the door open for reusing the same type on other signals and helps to align the type name across implementations.

See Java prototype: open-telemetry/opentelemetry-java#7123

@lmolkova lmolkova requested review from a team March 27, 2025 03:55
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.

Document that Extended Attribute term should be used to represent an extended set of standard attributes (if introduced by SDK).
This leaves the door open for reusing the same type on other signals and helps to align the type name across implementations.

This PR does not change anything in the https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/README.md. Therefore, it does not look like addressing the intend of this PR. The changes are only targeting Logs API and Data Model and to not say anything about the usability.

However, I want to call out that the Collector SIG said that for OTel Collector attributes are already what is defined as "Extended Attributes". See:

The Go SIG are also not in favor of creating another "extended attribute" type. See: open-telemetry/opentelemetry-go#6168 (comment)

Wouldn't such change be seen as breaking given the specification defined the map<string, any> type only in the scope of Logs and not a common type?

CC @open-telemetry/collector-maintainers @open-telemetry/go-maintainers

Comment on lines +465 to +466
Implementations that define a new type to represent extended attributes SHOULD use
the term `Extended Attributes`.
Copy link
Member

@pellared pellared Mar 27, 2025

Choose a reason for hiding this comment

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

What does "use the term" means? Moreover, why would we use a term that is not defined anywhere else?

@reyang
Copy link
Member

reyang commented Mar 27, 2025

It is unclear what problem this PR is trying to solve. Consider creating an issue and follow the instructions https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#proposing-a-change

@trask
Copy link
Member

trask commented Mar 27, 2025

When we look at how SDKs should embrace complex attributes on Logs there are multiple options, and it would be beneficial to the ecosystem if there is some degree of alignment across SDKs.

Much of this alignment just comes down to having consistent names to refer to things across languages.

Here are two options below. It would be great it if folks who support some other option can comment and briefly explain their preference.

1. Change standard attributes to include complex attributes

It sounds like this is what the Collector has done, but it contradicts existing specification language.

I believe @lmolkova and I both support this approach but there hasn't been enough spec support to explore overturning the existing language.

2. Introduce the concept of "Extended attributes" which is a superset of standard attributes and complex attributes

This is what this PR is aiming for and is what the Java SIG prefers (see prototype).

As @jack-berg says in slack

The name implies their purpose - an extended version of standard attributes. The name omits “log” keeping the door open for reuse if we want to bring the concept to other domains in the future.

@trask
Copy link
Member

trask commented Mar 27, 2025

The Go SIG are also not in favor of creating another "extended attribute" type

@pellared what is the Go SIG's preference? can you summarize here so we can add it to the above options that we are considering?

@pellared
Copy link
Member

pellared commented Mar 27, 2025

The Go SIG are also not in favor of creating another "extended attribute" type

@pellared what is the Go SIG's preference? can you summarize here so we can add it to the above options that we are considering?

I believe @lmolkova and I both support this approach but there hasn't been enough spec support to explore overturning the existing language.

I think we are in the same boat.

From the comment I referenced to:

It would be only truly reusable if there is a single attributes abstraction. If it is about adding support for nested attributes in existing signals (e.g. tracing) then it would be better to extend the standard attributes to avoid confusion and API design flaws (an API accepting both extended and standard attributes will be confusing for the user). However, it does not seem any longer acceptable per #3858

Extending the set of standard attribute value types is a breaking change.

Basically the result of #3858 made us define attributes as a separate type: https://pkg.go.dev/go.opentelemetry.io/otel/log#Value.

I think that the same happened for Rust: https://github.com/open-telemetry/opentelemetry-rust/blob/a071d8fc3906da56e089061927fbc1561e0e7677/opentelemetry/src/logs/record.rs#L61-L87

CC @cijothomas

@pellared
Copy link
Member

It sounds like this is what the Collector has done, but it contradicts existing specification language.

.NET kind of did this as well as the attribute value can be anything (object).

@lmolkova
Copy link
Member Author

lmolkova commented Mar 27, 2025

It is unclear what problem this PR is trying to solve. Consider creating an issue and follow the instructions https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#proposing-a-change

to add a practical aspect to the #4462 (comment)

If you take a look at the Python PR that attempts to implement support for extended attributes - open-telemetry/opentelemetry-python#4342 (comment) or similar Java PR - open-telemetry/opentelemetry-java#7123, both try to find the API name for this new thing (non-standard attributes) and they came up with two different names - LogAttributes and ExtendedAttributes. We need a common terminology in the spec to avoid such discrepancies.

A few SDKs (and collector) support extended attributes on the API surface already, so they don't necessarily have this problem. The rest of SDKs would need to align on the name.

To echo others: I would prefer to have one Attributes type that supports any value. But there is a lot of pushback and current spec language prevents it.

@pellared
Copy link
Member

pellared commented Mar 27, 2025

It would be great it if folks who support some other option can comment and briefly explain their preference.

When wearing the "OTel Spec Sponsor" hat:

3. Having "Extended attributes" only in Logs (and these could be named Log Attributes)

But there is a lot of pushback and current spec language prevents it.

If there were so many pushbacks from having one Attributes type that supports any value. Then I would predict that the same could happen for reusing any in other/future signals. Because why it was not simply extended initially?

From #3858 description:

While I was in favor of supporting complex attribute types (#2888) I believe its more important that we commit one way or the other. The uncertainty around the question of whether this type of evolution will occur has muddied the waters of several related conversations.

I think that we have the exact same uncertainty for future signals and adoption of extended attributes. Personally, I see adding a new "extended attributes" type as a hack for a possibly bad decision we have taken.

If there was a preference by the community that want to have the attribute types limited then we should support the "extended attributes" only for the Logs (for compatibility with existing logging systems).

If we want to "follow" the Collector design (which is also a preference of some part of the community) then we should simply extend the existing standard attributes type... However, given:

NOTE: extending the set of attribute value types is a breaking change.

I am not sure what the community would think about the project if we change the decision.

Also from #3858 (comment):

To me, its a question of whether we really want to firmly take complex attribute types off the table or not.

If we would like to change our decision then we should not take it lightly... Probably we should make some OTEP, user surveys, prototypes etc...

Side notes

I think that @tigrannajaryan was right here: #3858 (comment). I approved the PR as I just wanted to have "some decision" to move the OTel Go Logs API implementation further... When I am wearing the "OTel Go Maintainer" hat then I would like the standard attributes to be extended.

@lmolkova
Copy link
Member Author

lmolkova commented Mar 28, 2025

If we use LogAttributes or anything signal-specific, we'll have to do the same for profiles (or any future imaginary signals). There is no reason to restrict profiles to standard attributes.

Also, while the spec language is somewhat restrictive, all past discussions showed that we don't want to close the door for extended attributes on spans and see a lot of use for them in semantic conventions.

Let's imagine the evolution in this direction:

Scenario Future API Future Deprecations
0. now - LogAttributes
future - profiling and hypothetical signals
ProfilingAttributes -
1. now - LogAttributes
future - extended attributes on spans, but not metrics
ExtendedAttributes on spans and logs LogAttributes
2. now - LogAttributes
future - extended attributes on all signals
Attributes will have new methods LogAttributes
3. now - ExtendedAttributes
future - extended attributes on spans, but not metrics
- -
4. now - ExtendedAttributes
future - extended attributes on all signals
Attributes will have new methods ExtendedAttributes will merge into Attributes
5. now - extend Attributes for all signals with limitations for metrics - -

There is no future evolution scenario that would need LogAttributes to stay (unless we want per-signal attributes 🙈 ).

@lmolkova
Copy link
Member Author

lmolkova commented Mar 28, 2025

NOTE: extending the set of attribute value types is a breaking change.

I am not sure what the community would think about the project if we change the decision.

I do agree with the perception, but I don't think we should declare something breaking without leaving some proof of why and how it is breaking. And if we can prove it can be changed in non-breaking manner, we should consider this language a bug.

Update: I'm also worried about the perception if we stick to technically problematic decisions only because of a note like this.

@lmolkova
Copy link
Member Author

lmolkova commented Mar 28, 2025

A keep-the-status-quo solution for the practical problem could be that SDKs decide how to support it:

  • Allow any value on existing Attributes on the API level and drop complex values on signals that don't support them in the SDK. That's what .NET, Python and JS do today (with caveats). We can have a spec language that formalizes this behavior (as one of the possible ways). This would be the future-proof solution that can accommodate any spec changes.
  • Have different APIs (add ExtendedAttributes), take them on logs and future signals, provide some level of compatibility with Attributes. SIGs that do it should be ready to adjust this API depending on the spec evolution

@github-actions
Copy link

github-actions bot commented Apr 5, 2025

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 13, 2025
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants