open-telemetry / oteps

OpenTelemetry Enhancement Proposals
https://opentelemetry.io
Apache License 2.0
337 stars 164 forks source link

Introduce Scope Attributes #201

Closed tigrannajaryan closed 2 years ago

tigrannajaryan commented 2 years ago

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

See additional discussion here.

tedsuo commented 2 years ago

Are there prototypes available for review?

tsloughter commented 2 years ago

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 https://github.com/open-telemetry/oteps/pull/201#discussion_r858946170

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 commented 2 years ago

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 commented 2 years ago

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

@tsloughter yes. I made a clarification here.

tigrannajaryan commented 2 years ago

@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.

tsloughter commented 2 years ago

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

@tsloughter yes. I made a clarification here.

Ah, makes sense, a use case like distinguishing profiling logs from others would need to have name/version/schema_url the same but different attributes.

As is it is a simple enough addition to the Erlang/Elixir library. I'll worry about making it possibly more efficient and ergonomic later.

SWYZZWH commented 2 years ago

It's amazing that I propose an issue and find out your PR just perfectly solve my problem. Actually, I'm suggesting adding the "Attributes" field in Scope mainly because of performance consideration. In my case, our team is responsible for monitoring the status of millions of servers that generate billions of data points every second. To minimize payloads, I've designed a schema to describe the structure of monitoring data. The basic idea of the schema can be illustrated in the following figures. https://github.com/SWYZZWH/pictures/blob/main/metric_schema/freeschema2.png https://github.com/SWYZZWH/pictures/blob/main/metric_schema/freeschema.png

As a great part of metrics are collected by uniagent( a branch of otel-collector maintained by my company), my schematic data must suit otel-proto. While the majority are perfectly corresponsive, like target v.s. Resource, metric set v.s. instrumentation library, and metric v.s. Metric, tags of metric sets can't be stored in instrumentation library. I understand this may not be the appropriate nor standard way to utilize the proto. But I believe adding Attributes to instrumentation library does make the protocol more expressive and extendable. And I'm wondering why Resource has Attributes while instrumentation library doesn't. Both of them are just meta info from my perspective.

tigrannajaryan commented 2 years ago

Are there prototypes available for review?

@tedsuo Not a full prototype by any means, but here is a very quick draft that shows how WithScopeAttributes option can be added to the Tracer without breaking existing API: https://github.com/open-telemetry/opentelemetry-go/pull/2884

The Go maintainers may have much better proposals about this should be done properly. (cc @open-telemetry/go-maintainers)

tedsuo commented 2 years ago

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.

tigrannajaryan commented 2 years ago

@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 commented 2 years ago

Great, sounds good.

Oberon00 commented 2 years ago

@tedsuo I had similar thoughts at first: https://github.com/open-telemetry/oteps/pull/201#discussion_r863794422

While this is for sharing attributes that apply to all emitters, there are more "scopes" thinkable over which you could share attributes, e.g. https://github.com/open-telemetry/opentelemetry-specification/issues/335 / https://github.com/open-telemetry/opentelemetry-specification/issues/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 https://github.com/open-telemetry/opentelemetry-specification/issues/579)

tigrannajaryan commented 2 years ago

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

tigrannajaryan commented 2 years ago

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

tigrannajaryan commented 2 years ago

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

tigrannajaryan commented 2 years ago

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

tigrannajaryan commented 2 years ago

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

tigrannajaryan commented 2 years ago

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

tigrannajaryan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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