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

Clarify usage of "otel." attribute namespace #1640

Merged

Conversation

tigrannajaryan
Copy link
Member

I noticed developers adding their own attributes to this namespace
without going through the specification. We need to regulate this
namespace through the specification, just like we do it for other
semantic conventions.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

No objections, but what practical impact (aside from being a warning) do you foresee here? Do we need some prescription of how to handle the non-spec attributes that start with otel.?

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

Sounds reasonable to me but I also wonder about the practical implications as Yuri already stated.
Also, please mention this change in the changelog.

specification/common/attribute-and-label-naming.md Outdated Show resolved Hide resolved
specification/common/attribute-and-label-naming.md Outdated Show resolved Hide resolved
specification/common/attribute-and-label-naming.md Outdated Show resolved Hide resolved
OpenTelemetry specification. These are typically used to express OpenTelemetry
concepts in formats that don't have a corresponding concept.

For example `otel.library.name` attribute is used to record the instrumentation
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I wonder if you can call that an attribute. This string will only appear in the Jager/Zipkin exporters at which point its debatable if this is an attribute or another kind of key/value pair.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is supposed to be recorded in the same map (tags in Jaeger vocabulary) where we record attributes. So, it occupies the same namespace of attribute names. I am not sure if there is a better term for this, I am open to suggestions.

specification/common/attribute-and-label-naming.md Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

Oberon00 commented Apr 26, 2021

what practical impact (aside from being a warning) do you foresee here?

We might define an optional collector filter that discards/warns about unknown otel attributes? Although that might be useful only for testing and not an actual production deployment, as it would hamper forward compatibility.

@carlosalberto
Copy link
Contributor

carlosalberto commented Apr 27, 2021

Looks good to me! let's merge this once the feedback has been addressed ;)

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Overall LGTM, please resolve the comments :)

@tigrannajaryan
Copy link
Member Author

No objections, but what practical impact (aside from being a warning) do you foresee here?

No other impact. Just a warning to prevent people from polluting "otel." namespace.

Do we need some prescription of how to handle the non-spec attributes that start with otel.?

We do prescribe behavior for each individual attribute (e.g. otel.library.name mentioned in the text). I do not think we need a generic prescription.

One more thing we can do is to refactor the spec a bit and have all otel.* attributes listed and defined in one place (currently we have duplicated info about otel.library.* for Jaeger and Zipkin exporter, with exactly the same prescribed behavior). This refactoring can be done in a separate PR (not directly related to this PR).

@carlosalberto
Copy link
Contributor

@tigrannajaryan I think we are ready to merge this. Could you add a CHANGELOG entry before we do so?

I noticed developers adding their own attributes to this namespace
without going through the specification. We need to regulate this
namespace through the specification, just like we do it for other
semantic conventions.
@tigrannajaryan
Copy link
Member Author

@tigrannajaryan I think we are ready to merge this. Could you add a CHANGELOG entry before we do so?

Done.

@carlosalberto carlosalberto merged commit 5ad8cfb into open-telemetry:main Apr 28, 2021
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.

None yet

6 participants