-
Notifications
You must be signed in to change notification settings - Fork 889
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
Scope attributes as part of identity is a breaking change #2762
Comments
@bogdandrutu Can you point to the part of the spec which forbids this (before the change)? |
If we remove attributes from the identity of the Scope one consequence of that it is not possible to have a variety of Scopes that serve different purpose but are implemented in the same instrumentation library. For example I may need one Logger Scope that emits profiling events, and another Logger Scope that emits Entity events and yet another Logger Scope which emits debug log records. These are 3 different Scopes and to differentiate them we want to specify an attribute on the Scope (e.g. "otel.profiling=true"). However, the identity limitation forces are to implement these distinct functionalities in separate libraries. It is an undesirable limitation which artificially forces the instrumentation author to split their library into as many parts as many distinct concerns they may need to serve using one signal (e.g. using Log signal). So, longer term we may need to support both identifying and non-identifying attributes on the Scope (and this has been a discussion topic for Resources as well). For now, I think it is OK to support only non-identifying attributes. Note: I don't think recipients of telemetry should assume that non-identifying attributes are immutable. So, they should be ready to receive telemetry from the same Scope, identified by (name,version) tuple, but which has different attributes recorded. This is still the same Scope. |
Posting this one separate. There is also
If we believe this to be a spec bug then we should fix it. I don't think making this change can do any harm since the concept of schemas itself is considered experimental for now and changing semantics around |
You mention a change but link only to a file. I think the change you mean is this PR: #2579? I think scopes would be much less useful if only one is allowed per instrumentation library / version combination. We originally had this in the spec before this change, which explicitly allows returning different instances even for the same name+version+schema_url:
Now we have:
(which has a similar or maybe the same effect) So I don't think this was breaking. |
Sure, you have "name, version, schema_url" identifiers. So you have a backend that receives these values and treat them as identifier, hence they construct some logic to put this into a map (which guarantees that no duplicates will be received, since identifiers), if we allow to identify by attributes then I receive duplicates. Or let's get another example, I had a metric and I used (name/version/schema (actually schema is bad to be identifier but I would not start that)) as attributes to create unique timeseries into my resource, now with the new attributes I will break that "unique" so I will have conflicts. |
@bogdandrutu What would you suggest to do? Revert the addition of scope attributes? Only allow one scope per instrumentation library? Bump the spec version to 2.0? Add a warning in the changelog that this might break some use cases? |
😬 Is this an opportunity to declare OTLP 1.0?
I'd like to clarify exactly how this is forbidden. Informally yes, I agree with that term. In https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md at HEAD, we have:
The following section The problem in this case is that software built prior to recognition of scope attributes will pass through conflicts without knowing it and has no way to inform the user. Likewise, consumers that are unaware can unintentionally break the data. To be very concrete, for example, consumers will break the single-writer principle because two metrics at the client will conflict with (overwrite) each other at the consumer. |
@Oberon00 Suggest to make attributes "non identifier". |
I avoided this suggestion myself because I want these attributes to be identifying. 😁 |
I think we need both :-) Unsurprisingly this keeps coming up for different concepts (previously for Resource). Luckily in OTLP we have not defined whether attributes are identifying or non-identifying. So, because it was undefined previously we are formally free to define it now. Do note that this is similarly undefined for the Resource in OTLP. We don't make any claims about what attributes are identifying.
Well, to be honest, I don't know. I am not sure the wording Either way, I suggest the following to move forward:
|
So what does this mean if the same instrumentation library tries to create multiple scopes (with the same identifying parts)? Also, is this really an OTLP-problem, or a general data-model issue? |
We need to answer the following questions: Obtaining Tracer/Meter/LoggerIf within the same process session you use the Otel API to simultaneously obtain Tracer/Meter/Logger using the same (name,version) but different attributes what should happen? Do you get the same Tracer/Meter/Logger, or different Tracer/Meter/Logger or it is an error to try to do so or an undefined behavior? So 4 possible answers: same/different/error/undefined. I believe we want it to return the same. Does anyone disagree? Emitting ScopeIf over time, your process changes (e.g. restarts) and you obtain Tracer/Meter/Logger using the same (name,version) but attributes that are different from what you used to obtain it previously what should happen? The Otel API/SDK does not necessarily have a way to detect this (if the process is restarted there is memory of past Scopes retained) so what are our choices? I think the only choices we have is: a) it is an undefined behavior, b) it is a prohibited behavior but cannot be enforced, c) it is a normal behavior that is allowed and the emitted scope on the wire will use whatever attributes the caller used. I think it should be normal to obtain a Tracer/Meter/Logger with attributes that are different from historical values of the same Tracer/Meter/Logger as identified by its (name,version) pair. @bogdandrutu I think you are saying this should be prohibited behavior. If so can you clarify? |
I think what makes this whole conversation difficult for me at least is that outside of the two usecases in the OTEP I haven't seen any strong usecases for scope attributes and I don't really know what they are expected to be used for moving forward. The two usecases I've seen so far, from the OTEP, are adding a short name for the scope and adding a routing identifier to differentiate between logs and events. Both of those usecases seem like metadata to me and not identifying attributes. It seems like what we really wanted here was a place to describe the scope itself, and not necessarily the data it produces which is typically what attributes are used for. |
Before scope attributes it was specifically undefined if the same or a different meter/tracer/logger was returned when the same name/version/schema was used. Adding attributes shouldn't change that. |
I am saying that we should not say "attributes" are identifiable. changing a non identifiable attribute can be ok or not, but that is a separate topic. This issue is about your solution: "In the spec revert the unreleased change that says attributes are part of identity. This wasn't released and we consider it a mistake, a bug anyway, so treat it as an unreleased bug fix. Doesn't need a spec version bump or anything." |
I believe the point of contention was not about "attributes that are different from historical values of the same Tracer/Meter/Logger as identified by its (name,version) pair". It is about attributes that are different from other scopes within the same resource concurrently, right? |
That is my understanding as well. However, @bogdandrutu says it is a problem for receivers to see Scopes with the same (name,version) and different attributes, which if true means it needs to be prohibited in the API as well. I don't know if this is true, I'd like to understand if it is so, because "prohibition" may not be very useful since it is hard/impossible to enforce in the API and likely people will just end up violating such prohibition even if we add it to the spec. |
@dyladan there are/were 2 use cases that I am aware of:
|
Those were the same usecases I was aware of. Both of those seem like different concepts than what we have typically used attributes for in OTel and I would describe them more as metadata. I wasn't a part of the log record discussion but I'm very surprised an enum wouldn't have been used for that. |
I think it is fine to think about it as "metadata" of the Scope. However, I still see uses cases where you would want one instrumentation library to emit Scopes with different attributes. I brought the example above. So, IMO, long-term we should have a way to add more attributes to the Scope's identify. For now we can live with the limitation that Scope is identified by its name and version only.
We had very extensive multi-week discussions about event domain and made a call in the Log SIG. It is in the experimental part of the spec, so technically can be changed, but I think it is best to keep it out of this thread for now and assume that it is a legitimate use of Scope attributes. I do agree that it would be great to have more uses cases for attributes to understand the problem better. |
The instrumented library version may be another use case for a Scope attribute: #1605 |
Another example in open-telemetry/opentelemetry-js#3222 - for the browser instrumentation we are planning to have the current page url as a scope attribute. The current page url serves as the context for the spans and events created/emitted while the user is on that page in the browser. So, when a user navigates to a different page, there will be a new instance of the tracer/logger object - it will have a different value for this scope attribute but will be otherwise equivalent to the object in the previous page. |
Thanks for the additional usecase.
I see. The example linked here should definitely be started in the spec or the client instrumentation SIG by the way since it would apply to more than just web pages. It also seems to me that it would need to apply to all instrumentations and all signals in order to fulfill its true usefulness, rather than just the HTTP/xhr instrumentations. Your comment on the original issue implies you would rather have this be an ephemeral resource attribute and I would tend to agree. After seeing these and other usecases I would tend to think they should be a part of the identity. If they are not a part of the identity of the metric stream, the question becomes "what do you do with them," especially in backends that don't have support for non-identifying attributes. But I also agree with @bogdandrutu's point that this is actually a breaking change splitting identity where it would previously have been bonded. I think @jmacd's suggestion of one scope per library is a good one, but it does preempt some of the usecases listed in this issue. For example, if you can only output one scope per library then you can't have the page URL as @scheler wanted or the version as @tigrannajaryan linked in a separate issue. It also makes me ask where it is enforced. For example, is it only one scope per export payload, per process, etc? |
@jmacd is there a use case you have in mind for them being identifying? I'm fine either way, I think, I mainly want to use these attributes for semantic conventions -- for example, currently lots of duplicate attributes done on HTTP or database spans because attributes common to the connections have to be set on individual spans -- which don't need to be identifying. |
Instrumentation library is only one possible type of a Scope. Scopes are not limited to instrumentation libraries only. The definition of the Scope is the following:
Given this definition I think it is reasonable to claim that a webpage is a unit of application code. If we believe this is wrong then we need to change/refine the definition of the Scope to prohibit such usage more explicitly (I don't think we should prohibit). |
From my perspective, a lot of RUM shared-attributes seem more like "Baggage" and actually are ill-supported in our current APIs. Specifically, any automatic influence of Baggage to Trace/Metric/Log attributes or ability to attach scoped attributes to all signals generated is at odds. Instrumentation-Scope perhaps solve this, but with how instrumentation is generated today, I don't think it's common to construct a new scope per-actual-scope (e.g. I make a new Meter per-request that attaches may scoped attributes to it). I.e. It seems we should really think through these RUM use cases a bit more and how they actually manifest in our APis vs. how isntrumentation is writtten today. |
That seems inconsistent with the API which says that the scope name should be the instrumentation library name. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#get-a-tracer |
Where does it say that? Here is what I see in in the API:
|
Yes, that's what I meant. We seem to interpret this differently? My interpretation is that the name should identify the instrumentation (hence the name "instrumentation scope"). And the glossary also seems to define the instrumentation scope as associated with a unit of code. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/glossary.md#instrumentation-scope |
Thinking more about this I think it is a stretch to say that webpage is the Instrumentation Scope (so I tend to agree with @Oberon00). If you do this then you give up any opportunity to structure the Scopes based on the code structure (i.e. by library names or by JS module names, etc), which would be a lot more natural based on how the Scope is defined ("A logical unit of the application code with which the emitted telemetry can be associated"). I think @scheler your case really is different and @jsuereth is right, it may need a different solution. |
I just remembered, there was some discussion on the webpage use case in this OTEP that proposes a different solution: open-telemetry/oteps#208 |
I will wait for a TC member to join the RUM SIG. It's hard to discuss this topic over here. For now, you can exclude the RUM use-case for the topic in this issue. |
I don't feel strong one way or the other about this usecase but you could certainly have a UI framework or routing framework itself be an instrumented library and in that case the current page would be quite a reasonable scope attribute in my opinion |
I've been thinking about the following solution, here goes: |
Fixes open-telemetry#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes open-telemetry#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes open-telemetry#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes open-telemetry#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Resolves open-telemetry#2762 This change says that Scope attributes are NOT part of the Tracer/Meter/Logger identity. The consequence of this change is the following: - It is no longer possible to obtain a Logger with different sets of attributes from the same instrumentation library (using the same name, version,schema_url). It may be desirable to make this possible in the future, but right now it is impossible since the attributes are not part of the Scope's identity. - Likewise, it is no longer possible to obtain a Tracer and Meter with different sets of attributes from the same instrumentation library (using the same name, version,schema_url). This is likely the only desirable way for Tracer and Meter.
Resolves open-telemetry#2762 This change says that Scope attributes are NOT part of the Tracer/Meter/Logger identity. The consequence of this change is the following: - It is no longer possible to obtain a Logger with different sets of attributes from the same instrumentation library (using the same name, version,schema_url). It may be desirable to make this possible in the future, but right now it is impossible since the attributes are not part of the Scope's identity. - Likewise, it is no longer possible to obtain a Tracer and Meter with different sets of attributes from the same instrumentation library (using the same name, version,schema_url). This is likely the only desirable way for Tracer and Meter.
Fixes open-telemetry#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes open-telemetry#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes open-telemetry#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes #2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes open-telemetry/opentelemetry-specification#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Fixes open-telemetry/opentelemetry-specification#2762 The identity part can be changed later if the community agrees to accept this change, but to be safe for the moment revert this change. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
During today's SIG meeting, @tigrannajaryan point me to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter.
This is a breaking change, since after this change is applied as a receiver of telemetry I can receive now multiple scopes with same name/version which was forbidden before.
The text was updated successfully, but these errors were encountered: