-
Notifications
You must be signed in to change notification settings - Fork 163
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
Introduce Scope Attributes #201
Conversation
ef5cdeb
to
b2ff5ff
Compare
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).
b2ff5ff
to
998fc3b
Compare
Are there prototypes available for review? |
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 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. |
…s MUST return different Tracer
All, I made an important amendment to the OTEP. Now it says that "The implementation MUST NOT return the same 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 |
@tsloughter yes. I made a clarification here. |
@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. |
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:
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. |
@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. |
Great, sounds good. |
@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) |
We have a confirmation from Go maintainer that the OTEP is implementable in Go. |
@jack-berg since you approved can I assume that it means you think the OTEP is implementable in Java? |
@blumamir do you think the OTEP is implementable in a reasonably performant way in JS SDK? |
We have a confirmation from Java maintainers that this OTEP is implementable. |
We have a confirmation from Python maintainers that this OTEP is implementable. |
We have a confirmation from Ruby maintainers that this OTEP is implementable. |
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. |
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. |
We have had this open enough time and have enough approvals/confirmation. Let's merge. |
There are a few reasons why adding Scope attributes is a good idea:
represented as an attribute of Meter's Scope.
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.
LogRecord.
See additional discussion here.