[Common] Clarify Attribute support based on stable wire protocol definition of Attributes #2581

[MSNev]

Fixes # #2581 [Common] Spec inconsistency with proto definition of Attributes

Changes

• Clarifies the Attribute support as defined by Proto AnyValue and clarifies Attribute Collections.
• Clarifies and identifies how nested attributes support (as defined by Proto) should be supported.

~Related issues #~ ~#376 Support ~map values and~ nested values for attributes #376~

[github-actions[bot]]

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

[MSNev]

Not stale

[Aneurysm9]

Clarifies the Attribute support as defined by Proto AnyValue and clarifies Attribute Collections. Clarifies and identifies how nested attributes support (as defined by Proto and some SDK's) should be supported.

To the extent that this would apply to attributes for traces and metrics I think this goes beyond clarification and changes the specification. To my knowledge there are no trace/metric SDK implementations that allow nested attributes.

[MSNev]

Clarifies the Attribute support as defined by Proto AnyValue and clarifies Attribute Collections. Clarifies and identifies how nested attributes support (as defined by Proto and some SDK's) should be supported.

To the extent that this would apply to attributes for traces and metrics I think this goes beyond clarification and changes the specification. To my knowledge there are no trace/metric SDK implementations that allow nested attributes.

If you read the spec changes, I believe that I have not specified that trace/metric implementations have to implement or allow for nested attributes. And even the PR comment from above I am stating how it should be supported not that it must or is.

I've now removed the "and some SDK's" based on re-reviewing the JavaScript implementation and removing it from the support matrix.

What specific or additional clarification would you suggest?

[github-actions[bot]]

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

[Oberon00]

Please give this PR a more descriptive title. I think the "inconsistency" issue has been explained. It seems to me this is not solving an inconsistency but proposing a new feature.

[MSNev]

Please give this PR a more descriptive title. I think the "inconsistency" issue has been explained. It seems to me this is not solving an inconsistency but proposing a new feature.

Renamed.

I don't believe that this PR is identifying any new functionality, it is attempting to properly document what Attribute support is today.

• The protocol SUPPORTS nesting for Attributes (and has for around 2 years)
• Logs SUPPORTS and requires nested attributes

While Spans and Metrics also use "Attributes" their definition of Attributes does not support nesting (from at the spec level), which if not clarified is going to cause confusion not just at the consumer level (using the API vs Protocol) but also at the implementation level when SDK's start to support the proper definition of the Logs Attributes

[Oberon00]

I don't believe that this PR is identifying any new functionality, it is attempting to properly document what Attribute support is today.

In practice, you are proposing a new feature. This is not supported today.

• Python: https://github.com/open-telemetry/opentelemetry-python/blob/v1.11.1/opentelemetry-api/src/opentelemetry/attributes/init.py#L24-L57
• Java: https://github.com/open-telemetry/opentelemetry-java/blob/v1.16.0/api/all/src/main/java/io/opentelemetry/api/common/AttributeType.java

Do note that OTLP is merely a protocol that can be used for transporting (a superset of) OpenTelemetry-generated data. It is not the authoritative definition of the data model.

[MSNev]

In practice, you are proposing a new feature. This is not supported today.

That is because none of the SDK's (that I'm aware of) are currently SPEC compliant for LOGS, once they start to correctly implement Attributes for Logs everyone will hit this issue.

[Oberon00]

What should be "clarified" then, is that Log Attributes are not the general attribute type. You have a clear definition of a general "Attribute" here which does not allow nesting. You have a clear definition of the "attributes" field on log records at https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-attributes, which is of type map<string, any>, and you also have a definition of any that explicitly allows nesting at https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#type-any

[github-actions[bot]]

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

[MSNev]

What should be "clarified" then, is that Log Attributes are not the general attribute type.

That is what I'm attempting to do here without forcing the introduction of yet another "type", especially as this is unified at the protocol level.

Otherwise, this will cause potential unnecessary overhead for API and SDK implementations to be able to translate between types.

[github-actions[bot]]

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

[MSNev]

This last (forced) push is just to bring the PR up to date and mark as not stale. Forced push was required as git refused to do a normal push.

[github-actions[bot]]

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

[github-actions[bot]]

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

[MSNev]

Not stale as this is part of a larger issue like #2888

[MSNev]

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

Don't have the ability (permissions) to re-open...

[github-actions[bot]]

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

[github-actions[bot]]

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

[github-actions[bot]]

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