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

Define how providers handle instrumentation scope attributes and identical creation calls #4160

Closed
pellared opened this issue Jul 25, 2024 · 5 comments · Fixed by #4161
Closed
Assignees
Labels
area:api Cross language API specification issue area:sdk Related to the SDK spec:logs Related to the specification/logs directory spec:metrics Related to the specification/metrics directory spec:trace Related to the specification/trace directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned

Comments

@pellared
Copy link
Member

pellared commented Jul 25, 2024

Towards open-telemetry/opentelemetry-go#3368

Most of the SDKs ignore instrumentation scope attributes (C++ and PHP are exceptions). This is a functional blocker for following issues:

The specification does not explicitly stay how the SDK should handle tracer/meter/logger creation when the user calls it with the same identifying parameters (name, version, scheme_url), but when other parameters are different (currently only attributes).

Because of this the Go SDK ignores instrument scope attributes (see: open-telemetry/opentelemetry-go#3368).
I think the same is true for Java SDK, .NET SDK, JS SDK.

C++ SDK creates only a single tracer for many "identical" creation calls (the instrumentation scope attributes value comes from the first "identical" tracer creation call). However, it is only for OPENTELEMETRY_ABI_VERSION_NO=2 which is experimental.

Python SDK, PHP SDK create always a new instance of a tracer for many "identical" creation calls (with potentially different instrumentation scope attributes).

Right now, the specification does not require the SDK to handle scope attributes. Currently, having them ignored is specification compliant.

It is also unclear why scope attributes are not taking part of being one of the "identifying fields" and why all fields cannot be used to identify an instrument. From my database experience adding a column to a primary key it is an acceptable change/migration. However, in order to include instrumentation scope attributes to the identifying fields we would need to define the equality of Attribute Collections. In my opinion, the order or attributes should not affect the equality of attribute ([k1: v1, k2: v2] should be equal to [k2: v2, k1: v1]), especially that some languages model the attributes as map/hashset.

"Identical" and "identifying fields" terms are (currently) relevant and used a lot in metrics' duplicate instrument registration section.

Prior discussions: #4146

@pellared pellared added area:api Cross language API specification issue area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory labels Jul 25, 2024
@pellared
Copy link
Member Author

pellared commented Jul 25, 2024

I made a little draft/POC PR #4161.

I can look into it further (e.g. make a prototype in Go SDK) no sooner than on September. However, I may be more effective if someone who has been working more on the metrics specs/SDKs would take this issue after me (or with me).

@danielgblanco danielgblanco added the triage:deciding:tc-inbox Needs attention from the TC in order to move forward label Jul 29, 2024
@carlosalberto
Copy link
Contributor

From the TC call last week: we think the current behavior described in the Specification can be considered a bug or an error, and we would like to recommend that getTracer/getMeter/getLogger calls return instances reflecting the actual passed attributes in every call.

However, we want to verify with SIGs that this is fine, so users do not suffer because of the behavior change.

@lzchen
Copy link
Contributor

lzchen commented Jul 29, 2024

The specification does not explicitly stay how the SDK should handle tracer/meter/logger creation when the user calls it with the same identifying parameters (name, version, scheme_url), but when other parameters are different (currently only attributes).

I think the spec does not explicitly state how SDK should handle tracer/meter/logger creation even with the same identifying parameters (name, verison, schema_url) even WITHOUT differing attributes correct? (from the spec -> When more than one Tracer of the same name, version, and schema_url is created, it is unspecified whether or under which conditions the same or different Tracer instances are returned.).

Because of this the Go SDK ignores instrument scope attributes

Just a small correction to the above, Python SDK does support scope attributes for tracer/meter/logger creation with similar implementation details to PHP SDK in which we creating multiple instances for the same "identifying call".

@pellared
Copy link
Member Author

Just a small correction to the above, Python SDK does support scope attributes for tracer/meter/logger creation with similar implementation details to PHP SDK in which we creating multiple instances for the same "identifying call"

Thanks. Description updated.

I also discovered that the C++ behavior and API is experimental which is available when OPENTELEMETRY_ABI_VERSION_NO=2 is defined.

@pellared pellared changed the title Define how providers handle identical creation calls Define how providers handle instrumentation scope attributes and identical creation calls Jul 30, 2024
@tigrannajaryan tigrannajaryan added triage:accepted:needs-sponsor Ready to be implemented, but does not yet have a specification sponsor and removed triage:deciding:tc-inbox Needs attention from the TC in order to move forward labels Aug 21, 2024
@pellared
Copy link
Member Author

pellared commented Sep 3, 2024

@open-telemetry/technical-committee, I can be a sponsor as I am already working on this. See #4161

@trask trask added triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned and removed triage:accepted:needs-sponsor Ready to be implemented, but does not yet have a specification sponsor labels Sep 3, 2024
carlosalberto pushed a commit that referenced this issue Oct 9, 2024
…fields (#4244)

Small follow up to
#4161

## Changes

Updates the glossary to indicate that instrumentation scope's schema url
and attributes are also identifying. I simplified the language a bit.


* [x] Related issues #4160
@austinlparker austinlparker moved this to Spec - Closed in 🔭 Main Backlog Oct 22, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
…fields (open-telemetry#4244)

Small follow up to
open-telemetry#4161

## Changes

Updates the glossary to indicate that instrumentation scope's schema url
and attributes are also identifying. I simplified the language a bit.


* [x] Related issues open-telemetry#4160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue area:sdk Related to the SDK spec:logs Related to the specification/logs directory spec:metrics Related to the specification/metrics directory spec:trace Related to the specification/trace directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Projects
Status: Spec - Closed
6 participants