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

Explain why custom attributes are not recommended to be placed in Otel namespaces #3507

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented May 15, 2023

Use normative "RECOMMENDED" and add a sentence explicitly reinforcing that custom attributes don't belong in Otel namespaces.

This was the intent behind the rules and I think we should be allowed to make this clarification even though the doc is marked "Stable".

This is following from a discussion here where we noticed the current spec is not properly reflecting the intent: #3497 (comment)

[UPDATE] The @open-telemetry/technical-committee discussed and decided to keep the
existing recommendations but clarify them and explain the purpose.

Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

  1. we do not have registry of all OTel namespaces users can check against
  2. we cannot guarantee that namespace xyz user introduces will never be added to otel semconv
  3. it would require all more-than-one-company-semconv in the world to be registered with otel spec
  4. it does not seem user-friendly. If I want to enrich http span with http.content.type, why would otel want to limit my ability to do so?

What's the benefit (except for schema transformation clarity) in this limitation?

@tigrannajaryan
Copy link
Member Author

@lmolkova thanks for commenting.

  1. we do not have registry of all OTel namespaces users can check against

Isn't semconv repo that registry?

  1. we cannot guarantee that namespace xyz user introduces will never be added to otel semconv

True, not guaranteed, but if they follow the recommendations and use company name or application name as a prefix it will be quite unlikely. Perhaps we need to do more to provide stronger guarantees.

  1. it would require all more-than-one-company-semconv in the world to be registered with otel spec

Not necessarily. If all custom attributes are required to be placed in a predefined namespace that can't clash with Otel namespaces then we don't need a registry, a rule is sufficient. The rules that we have may or may not be enough, so perhaps we need to refine them.

  1. it does not seem user-friendly. If I want to enrich http span with http.content.type, why would otel want to limit my ability to do so?

You can do it at your own risk. The risk is that your definition will later conflict with Otel's definition of http.content.type, should Otel decide to add one. This is a bad situation to be in and is one of the problems we try to avoid by asking the users to avoid Otel namespaces for attributes they invent. The current recommendation for something like http.content.type is to propose it to Otel semconv instead of silently using it internally. This is the applicable part:

- The name may be generally applicable to applications in the industry. In that
  case consider submitting a proposal to this specification to add a new name to
  the semantic conventions, and if necessary also to add a new namespace.

What's the benefit (except for schema transformation clarity) in this limitation?

It prevents the users from ending up with attributes that have a different meaning in their own company vs the rest of the world that uses Otel semconv that is introduced after they decided to use a custom attribute name that falls in an Otel namespace.

Based on your comment it looks like you are against the currently defined recommendation in the Otel spec. What do you suggest to do instead?

@lmolkova
Copy link
Contributor

lmolkova commented May 16, 2023

we do not have registry of all OTel namespaces users can check against

Isn't semconv repo that registry?

So the algorithm for users would be to go search all yaml files in the semconv repo, find all namespaces and not use them? It sounds really difficult

we cannot guarantee that namespace xyz user introduces will never be added to otel semconv

True, not guaranteed, but if they follow the recommendations and use company name or application name as a prefix it will be quite unlikely. Perhaps we need to do more to provide stronger guarantees.

There are a lot of projects/libraries out there which come up with their own namespaces per project: camel, dapr. we have bunch or contrib code in collector setting attributes for docker, Azure/AWS/GCP.

I don't think the assumption that all custom attributes can be prefixed with company/project name is a valid one. Or that all of such attributes would be registered in the otel (so far very small portion of such attributes is actually brought up to this repo)

Based on your comment it looks like you are against the currently defined recommendation in the Otel spec. What do you suggest to do instead?

that's correct I do not support the limitation. Before I can come up with suggestions, Can you explain the problem you want to solve? From my point of view, there is no problem.
If user set a custom attribute value, it should not be dropped. If later on the same attribute is introduced in the spec, user will eventually onboard onto the new version of the spec. In the meantime, they won't get the best experience from their backend and that's enough of a punishment. No reason to be strict or cruel to our users.

@tigrannajaryan
Copy link
Member Author

Based on your comment it looks like you are against the currently defined recommendation in the Otel spec. What do you suggest to do instead?

that's correct I do not support the limitation. Before I can come up with suggestions, Can you explain the problem you want to solve? From my point of view, there is no problem.

@lmolkova Sorry, I wasn't clear, let me try again.

The spec currently recommends using custom attribute names such that they are prefixed with company name or app name and recommends creating a semconv should a name be needed that is generic enough to qualify for a place in Otel semconv namespace, and also has with some language that says "make sure names don't clash with semconv ...". The PR I submitted tries to clarify that, make more explicit and use normative language.

You say you disagree with this limitation. As far as I see the limitation is already in the "Stable" document, in a somewhat weak form, not very clearly written, but it is there. So, I am confused as to what you suggest to do given that you seem to disagree with a stable part of the spec. Do you suggest that we ignore the recommendations of the spec or are you reading the spec differently?

@lmolkova
Copy link
Contributor

lmolkova commented May 17, 2023

@tigrannajaryan I see this section as a soft recommendation I can decide to follow or not that does not try to tell me which behavior is invalid.

If I understand correctly, you want to make it stricter to justify dropping or modifying custom attributes.
The current version of this spec would not allow it and I'm happy with it.

Your proposal opens the door to such changes. It would also require us, as the community to create a registry of attributes and put more effort into registering namespaces for things that already live somewhere else. Without such effort, the requirement you're adding is not possible to follow in practice.
It's also not enforceable, so essentially you let users (most of whom will never read this part of the spec) to shoot themselves in the foot by adding intuitive attributes which then, with some future version of otel, might be modified without them knowing.

@lmolkova
Copy link
Contributor

lmolkova commented May 17, 2023

an easy way to be clear is to answer the question I have to ask again: what's the benefit of introducing this strict limitation?

It prevents the users from ending up with attributes that have a different meaning in their own company vs the rest of the world that uses Otel semconv that is introduced after they decided to use a custom attribute name that falls in an Otel namespace.

this change does not prevent anything, just allows us to blame them when conflict happens.

@yurishkuro
Copy link
Member

this change does not prevent anything, just allows us to blame them when conflict happens.

@lmolkova I see it as a way of providing guardrails for users not to find themselves in a situation where future OTEL attributes clash with the ones users are already using. OTEL has no way of knowing if such clash occurs by introducing new attributes, whereas users do have a way of checking against existing "official" namespaces (a grep against semconf yaml files is not very difficult, and we can even auto-generate that list as part of the docs). Using uppercase normative language simply means "please pay attention to this", since you're correct that we cannot enforce anything. It's better to have the expectations explicitly stated.

@tigrannajaryan
Copy link
Member Author

We have just had a discussion and @reyang had an interesting suggestion to allow custom attributes to be any name that is not using any namespace at all (i.e. has no dots in the name). Combined with the fact that Otel semconv will always be in some namespace it ensures that custom attributes never conflict with Otel semconv.

I like this because it gives freedom to end users to use short, meaningful names that makes sense in the context of their organization.

We can still allow the alternate option to prefix the custom attribute names by reverse FQDN, but it is less readable and is probably an overkill for most in-house attributes.

@lmolkova
Copy link
Contributor

lmolkova commented May 17, 2023

@yurishkuro there is little-to-no harm when it happens and there is no need to enforce it. The chance of collision is small anyway and when a collision happens, the data provided by user should have higher priority than spec compliance anyway.

when picking an attribute name as a user, I can come up with oci.manifest.digest - there is no collision yet, but then someone can contribute oci namespace to otel introducing collision without me knowing. And people do not have to contribute their attribute names to OTel spec and even when they do, these PRs don't often get merged.

@lmolkova
Copy link
Contributor

lmolkova commented May 17, 2023

@tigrannajaryan

Based on my experience and looking into how people use OTel beyond OTel (and even within) repos, I do not believe that any subtle rule we come up with for application developers will be followed and we have to embrace it.

@jsuereth
Copy link
Contributor

I'm with @lmolkova here. I think the only requirements we can place are on ourselves and otel compliant implementations.

This is an unenforceable restriction, and would only get used to point fingers.

Additionally there need be no restriction on user generated telemetry as it won't have a schema file associated.

It's ok for us to recommend how to avoid conflicts but I'd consider that non normative language.

@pyohannes
Copy link
Contributor

I'm with @lmolkova here. I think the only requirements we can place are on ourselves and otel compliant implementations.

I also agree. In my understanding, this proposal amounts to saying that every application developer adding attributes to telemetry that has a schema set needs to be familiar with the OpenTelemetry specification and follow the recommendations it contains, otherwise they might face unexpected consequences.

Most application developers I work with are at best familiar with the documentation of the respective language APIs/SDKs, but they have never looked at the OpenTelemetry specification. Requiring them to do so would be a big ask.

In my opinion, this makes sense as a non-normative recommendation in the documentation of APIs. Having it in the specification, it will likely not reach the target audience of application developers.

@Aneurysm9
Copy link
Member

I mentioned today in the Maintainers' meeting that I thought Elastic Common Schema had defined a reserved app. namespace for attributes, but I think I was misremembering and that was a policy we had layered on top of use of ECS on a prior system where we used it. ECS does, however, have language around conflicts and suggestions for avoiding them that may be useful to us here.

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Requesting changes to denote my agreement w/ @lmolkova. The current language is un-enforcable.

  • Please remove "You SHOULD NOT add new attribute names to OTEL namespace and use them"
  • Please mark existing guidance on conflcits w/ user telemetry as guidance, but non-normative.
  • If you want to update the use of names + namespaces to ONLY apply to OpenTelemetry provided experimental telemetry, I'd be for that.

@tigrannajaryan
Copy link
Member Author

The @open-telemetry/technical-committee discussed and decided to:

  • Avoid using strong normative language.
  • Keep the recommendations but clarify them and explain the purpose.

I will modify the PR to reflect this.

…l namespaces

The @open-telemetry/technical-committee discussed and decided to keep the
existing recommendations but clarify them and explain the purpose.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/clarify-custom-names branch from 45d29be to c10485c Compare May 24, 2023 17:19
@tigrannajaryan tigrannajaryan changed the title Reinforce that custom attributes should not be placed in Otel namespaces Explain why custom attributes are not recommended to be placed in Otel namespaces May 24, 2023
@tigrannajaryan tigrannajaryan dismissed stale reviews from jsuereth and lmolkova May 24, 2023 17:21

PR changed according to TC decision

@tigrannajaryan tigrannajaryan merged commit ce2c594 into open-telemetry:main May 26, 2023
7 checks passed
@tigrannajaryan tigrannajaryan deleted the feature/tigran/clarify-custom-names branch May 26, 2023 15:23
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