Remove scope attributes form the Scope identity

[bogdandrutu]

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 [email protected]

[MrAlias]

If scopes have attributes, but these attributes are not identifying, how will we resolve two providers with the same identity (same name, version, etc.) but different attributes? Does this mean OTel implementations should not add attributes to scopes?

[bogdandrutu]

If scopes have attributes, but these attributes are not identifying, how will we resolve two providers with the same identity (same name, version, etc.) but different attributes?

Providers? If you are talking about providers then they have different resource.

Meters/Tracers/Loggers? Then I think, since is clearly stated that this is not valid and undefined, an valid implementation may return the same instance as the first call and ignore attributes from the second call (may also report an error via the error reporting mechanism implemented, log, etc.), another approach is to return a no-op implementation second time and report an error.

Does this mean OTel implementations should not add attributes to scopes?

You should, most likely I miss something because I don't understand where this question is coming from.

[MrAlias]

If scopes have attributes, but these attributes are not identifying, how will we resolve two providers with the same identity (same name, version, etc.) but different attributes?

Providers? If you are talking about providers then they have different resource.

🤦, yeah, sorry I wrote that in error. I did mean Meters/Tracers/Loggers.

Meters/Tracers/Loggers? Then I think, since is clearly stated that this is not valid and undefined, an valid implementation may return the same instance as the first call and ignore attributes from the second call (may also report an error via the error reporting mechanism implemented, log, etc.), another approach is to return a no-op implementation second time and report an error.

Hmm, that doesn't seem ideal. That could have some implementations returning the original silently while others completely fail to record things.

[tigrannajaryan]

Hmm, that doesn't seem ideal. That could have some implementations returning the original silently while others completely fail to record things.

I think that's the point of undefined behavior. Users should not do this. If they do then then bad things can happen (including crashing, although I suppose we won't do it).

This is just for now. We can refine this behavior and specify what should actually happen. But because we are not yet sure what is the desirable behavior we are making it undefined in the spec and explicitly say that it is a user error.

[jmacd]

@bogdandrutu I agree with the undefined behavior, however of following options you listed for a valid implementation:

may return the same instance as the first call and ignore attributes from the second call

That sounds OK. There is no change of identity by definition and so equivalent telemetry data is produced. Users of the SDK that makes this choice will likely press for work on the specification to enable identifying scope attributes.

another approach is to return a no-op implementation second time and report an error.

That sounds NOT OK. You can tell the user there was a problem, but to stop reporting telemetry is a grave mistake.

Here's another behavior I believe is OK:

3. Return the intended scope with different attributes than others prior, therefore produce "conflicting" scopes. This is in line with existing behavior specified for SDKs, namely to pass through Metric data for all instrument definitions regardless of conflicts. Users of the SDK that makes this choice will likely press their vendors to recognize (and submit PRs to accomplish) the identifying nature of scope attributes without confirmation from the specification.

[MrAlias]

Hmm, that doesn't seem ideal. That could have some implementations returning the original silently while others completely fail to record things.

I think that's the point of undefined behavior. Users should not do this. If they do then then bad things can happen (including crashing, although I suppose we won't do it).

This is just for now. We can refine this behavior and specify what should actually happen. But because we are not yet sure what is the desirable behavior we are making it undefined in the spec and explicitly say that it is a user error.

Gotcha. But this leads me back to my original question of if an implementation should even add attributes to a scope at this point. If they aren't exported, cause undefined behavior if defined at creation of a Meter/Tracer/Logger, and make the scope an invalid map key to use for identity, why add them?

[tigrannajaryan]

Gotcha. But this leads me back to my original question of if an implementation should even add attributes to a scope at this point. If they aren't exported, cause undefined behavior if defined at creation of a Meter/Tracer/Logger, and make the scope an invalid map key to use for identity, why add them?

They only cause problem if you obtain Meter/Tracer/Logger with a different set of attributes, which is outlawed by this PR. If used correctly (always use the same attributes for the same Scope identity) then it will/should work fine. It should export without problems.

[MrAlias]

3. Return the intended scope with different attributes than others prior, therefore produce "conflicting" scopes. This is in line with existing behavior specified for SDKs, namely to pass through Metric data for all instrument definitions regardless of conflicts. Users of the SDK that makes this choice will likely press their vendors to recognize (and submit PRs to accomplish) the identifying nature of scope attributes without confirmation from the specification.

If this is a valid implementation, it seems fine to me. @bogdandrutu is this behavior intended to be allowed?

[bogdandrutu]

Return the intended scope with different attributes than others prior, therefore produce "conflicting" scopes. This is in line with existing behavior specified for SDKs, namely to pass through Metric data for all instrument definitions regardless of conflicts. Users of the SDK that makes this choice will likely press their vendors to recognize (and submit PRs to accomplish) the identifying nature of scope attributes without confirmation from the specification.

If this is a valid implementation, it seems fine to me. @bogdandrutu is this behavior intended to be allowed?

I would say that for the moment any implementation that does not make an assumption or needs the attributes to be identifiers is ok. I prefer to avoid the "conflicting" scopes proposal, since if that is available and start to be used will become part of your stability guarantees and you will not be able to change if the specification changes in a way to define that behavior, which is OK to do for the specification when UB is defined.

You should explicitly mark that as undefined behavior and tell users that can produce unspecified behaviors.

[MrAlias]

Return the intended scope with different attributes than others prior, therefore produce "conflicting" scopes. This is in line with existing behavior specified for SDKs, namely to pass through Metric data for all instrument definitions regardless of conflicts. Users of the SDK that makes this choice will likely press their vendors to recognize (and submit PRs to accomplish) the identifying nature of scope attributes without confirmation from the specification.

If this is a valid implementation, it seems fine to me. @bogdandrutu is this behavior intended to be allowed?

I would say that for the moment any implementation that does not make an assumption or needs the attributes to be identifiers is ok. I prefer to avoid the "conflicting" scopes proposal, since if that is available and start to be used will become part of your stability guarantees and you will not be able to change if the specification changes in a way to define that behavior, which is OK to do for the specification when UB is defined.

You should explicitly mark that as undefined behavior and tell users that can produce unspecified behaviors.

gotcha 👍