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

Introduce Scope Attributes #201

Conversation

tigrannajaryan
Copy link
Member

There are a few reasons why adding Scope attributes is a good idea:

  • There are 2 known use cases where Scope attributes can solve specific problems:
    • Add support for Meter "short_name",
      represented as an attribute of Meter's Scope.
    • Add support for differentiating the type of data emitted from the scopes that belong
      to different data domains, e.g. profiling data emitted as log records or client-side
      data emitted as log records needs to be differentiated so that it can be easily
      routed and processed differently in the backends. We don't have a good way to handle
      this today. The type of the data can be recorded as an attribute Logger's Scope.
  • It makes Scope consistent with the other primary data types: Resource, Span, Metric,
    LogRecord.

See additional discussion here.

@tigrannajaryan tigrannajaryan requested a review from a team April 25, 2022 22:46
There are a few reasons why adding Scope attributes is a good idea:
- There are 2 known use cases where Scope attributes can solve specific problems:
  - Add support for [Meter "short_name"](open-telemetry/opentelemetry-specification#2422),
    represented as an attribute of Meter's Scope.
  - Add support for differentiating the type of data emitted from the scopes that belong
    to different data domains, e.g. profiling data emitted as log records or client-side
    data emitted as log records needs to be differentiated so that it can be easily
    routed and processed differently in the backends. We don't have a good way to handle
    this today. The type of the data can be recorded as an attribute Logger's Scope.
- It makes Scope consistent with the other primary data types: Resource, Span, Metric,
  LogRecord.

See additional [discussion here](open-telemetry/opentelemetry-specification#2450).
text/0201-scope-attributes.md Outdated Show resolved Hide resolved
@tedsuo
Copy link
Contributor

tedsuo commented Apr 26, 2022

Are there prototypes available for review?

text/0201-scope-attributes.md Show resolved Hide resolved
text/0201-scope-attributes.md Show resolved Hide resolved
text/0201-scope-attributes.md Show resolved Hide resolved
@tsloughter
Copy link
Member

I assume different attributes, or no attributes, means it is a different Tracer?

My only concern with this is more related to implementation in languages without globals where sharing a Tracer variable isn't possible. Having to supply the attributes every time get_tracer is called would hurt the purpose as Tigran notes #201 (comment)

If attributes are treated the same as name/version/schema_url then it may be useful for us to have another operation that is for "registering" a tracer or scope so the attributes can be only set once. But this may be more implementation specific and not a concern for the spec itself.

@tigrannajaryan
Copy link
Member Author

All, I made an important amendment to the OTEP.

Now it says that "The implementation MUST NOT return the same Tracer when called repeatedly with different values of parameters."

This is necessary, otherwise our use cases will not work.

Note that this is not a breaking change. Previously it was an undefined behavior, now it is defined more precisely. It is acceptable to change undefined behavior into defined and it is not a breaking change since the caller were not allowed to rely on any undefined behavior.

cc @bogdandrutu @yurishkuro @alanwest @scheler @arminru @blumamir @jack-berg

@tigrannajaryan
Copy link
Member Author

I assume different attributes, or no attributes, means it is a different Tracer?

@tsloughter yes. I made a clarification here.

@tigrannajaryan
Copy link
Member Author

@bogdandrutu @yurishkuro @alanwest I made an important change in a new commit. I will re-request your review to make sure you saw the change.

@tedsuo
Copy link
Contributor

tedsuo commented May 4, 2022

Hi @tigrannajaryan.

I really like this proposal; it's clean, simple, and introduces a new context that I think we will really benefit from. Among other things, configuration attributes for things like database clients could potentially be reported as scoped attributes, which could be a very valuable input to automated analysis.

I've also reviewed this OTEP from the perspective of client-side (RUM) attributes such as SessionID, UserID, etc. I don't think these attributes could be placed within instrumentation scopes without adding additional complexity. Potential approaches for integrating scopes with something like a SessionManager:

  • Scopes could be nested, and the SessionManager could have access to the parent scope. Scope attributes would probably have to be mutable so that values in the parent scope could be changed without breaking the link between child and parent scopes.
  • Scopes could remain flat, and something like a ScopeAttributeProvider could need to be handed out everywhere so that the SessionManager could trigger an update of the SessionID attribute in all Scopes. This is essentially the same as the ResourceProvider concept, except the same data would be duplicated across many scopes.

I'm reluctant to suggest adding this complexity here. I think your current proposal is elegant, and there is no need for this additional complexity when all scope attributes are supplied locally by the instrumentation. Even in the case of instrumentation which may want to update it's scope attributes, there's no need for a ScopeAttributeProvider, as the instrumentation can just recreate the tracer/logger/etc.

All of the additional complexity involved in storing a SessionID as a scope attribute comes from the need for non-local attributes to be mixed into the scope. We wouldn't want every instrumentation library to be forced to take something like a SessionManager as an input, not to mention the additional code needed to recreate tracers and loggers whenever the SessionManger updates. A ScopeAttributeProvider would centralize some of this complexity, but I don't think it's an improvement over a ResourceProvider.

Here's why I don't think it would be an improvement: conceptually, scopes represent a localized context. Pushing non-local context like SessionID into every scope violates that concept, and creates additional complexity as a side effect. If we have to add some complexity to OTel in order to manage sessions properly, I'd prefer we do that without mixing non-local attributes into a local context. In our data model, I believe that Resources represent the correct context for things like SessionID and UserID in mobile/web clients. The fact that these attributes would not need to be duplicated if they were resources is an indicator that this is the correct context to place them in.

Having to wrangle with the concept of updatable resources is unfortunate, but I'm concerned that our data model would lose clarity while retaining much of the same complexity if we tried to use scopes for this purpose.

Hope that analysis is helpful. We can continue this conversation as part of the RUM SIG, but if you're satisfied that scopes are not the right answer, I would suggest we approve this OTEP and not the sessionID issue slow down the implementation of scope attributes.

@tigrannajaryan
Copy link
Member Author

@tedsuo I agree with you.

The Scopes attributes as they are defined by this OTEP are not a very good fit for capturing the session id. The Scope attributes API defined by this OTEP is targeted for use-cases, which know upfront, at the time of Tracer/Meter/LogEmitter creation what the attributes values should be and sets them once and for the entire lifetime of the Tracer/Meter/LogEmitter.

The session id appears to be a different beast. It can change any time and these changes are not synchronized in any way to the lifetime of the Tracer/Meter/LogEmitter as it is understood by Otel APIs today.

I think you do need a different mechanism for session ids. Perhaps the right framing for session id is "enrichment" or "on the fly modification". All emitted telemetry is enriched by an attribute called "session_id", the value of which can be provided by a user-defined function. From this perspective it appears the notion of the "Provider" that you suggest can be the right approach.

I think we can continue the session id discussion in its own thread (your doc or in a new OTEP). I am happy to keep this OTEP open for a while. I want to make sure we find a good, different solution for sessions ids, a solution which does not rely on Scope Attributes and does not require changes to this OTEP.

@tedsuo
Copy link
Contributor

tedsuo commented May 4, 2022

Great, sounds good.

@Oberon00
Copy link
Member

Oberon00 commented May 5, 2022

@tedsuo I had similar thoughts at first: #201 (comment)

While this is for sharing attributes that apply to all emitters, there are more "scopes" thinkable over which you could share attributes, e.g. open-telemetry/opentelemetry-specification#335 / open-telemetry/opentelemetry-specification#1143 would probably call for sharing attributes on some subtree of a trace, or on a subset of spans created by an telemetry producer (e.g. something like open-telemetry/opentelemetry-specification#579)

@tigrannajaryan
Copy link
Member Author

We have a confirmation from Go maintainer that the OTEP is implementable in Go.

@tigrannajaryan
Copy link
Member Author

@jack-berg since you approved can I assume that it means you think the OTEP is implementable in Java?

@tigrannajaryan
Copy link
Member Author

@blumamir do you think the OTEP is implementable in a reasonably performant way in JS SDK?

@tigrannajaryan
Copy link
Member Author

We have a confirmation from Java maintainers that this OTEP is implementable.

@tigrannajaryan
Copy link
Member Author

We have a confirmation from Python maintainers that this OTEP is implementable.

@tigrannajaryan
Copy link
Member Author

We have a confirmation from Ruby maintainers that this OTEP is implementable.

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented May 20, 2022

I think we now have sufficient confirmations from many maintainers to be confident that the OTEP is implementable.

If you still have any concerns please comment.

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented May 20, 2022

I resolved all comments. I will keep this open for a bit longer to give people time to object. Unless I hear objections I will merge it.

@carlosalberto
Copy link
Contributor

We have had this open enough time and have enough approvals/confirmation. Let's merge.

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.