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.

Sign up for GitHub

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

Jump to bottom

Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger #4161

Merged
merged 19 commits into from
Sep 24, 2024
Merged

Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger #4161

merged 19 commits into from
Sep 24, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Jul 25, 2024 •
edited
Loading

Fixes #4160 (prior art: #4146)

Python SDK, PHP SDK look to be compliant with the proposal. AFAIK these are the only languages which handle scope attributes in their stable releases.

Additional note: I do not want to merge it before I prototype it in the Go SDK. However, I want to have some "acceptance" before writing the prototype. PoC in Go: open-telemetry/opentelemetry-go#5806

Remarks: I do not feel competent to handle similar issue for instruments in metrics. Especially how to refine the duplicate instrument registration section. I decided to leave this section untouched.

@pellared pellared mentioned this pull request Jul 25, 2024
Closed
@pellared pellared changed the title [POC] Remove identifying fields term [POC] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Jul 25, 2024
@pellared pellared changed the title [POC] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Jul 25, 2024
@pellared
Copy link
Member Author

@yurishkuro, you can take a look if this is inline with your #4146 (comment)

yurishkuro
yurishkuro approved these changes Jul 25, 2024
View reviewed changes
specification/logs/sdk.md Outdated Show resolved Hide resolved
specification/logs/bridge-api.md Outdated Show resolved Hide resolved
@pellared
Copy link
Member Author

PTAL @jmacd @tigrannajaryan

@tigrannajaryan
Copy link
Member

@open-telemetry/specs-approvers

This is a change in Stable docs. We need to decide if it is allowed.

We are essentially saying that a behavior that was previously a user error (repeatedly obtain Tracer/Meter/Logger with the same name/version/schema_url and different attributes) is now valid.

Can we treat this as a backward compatible, non-breaking enhancement that fits our stability guarantees?

@pellared
Copy link
Member Author

@open-telemetry/specs-approvers

This is a change in Stable docs. We need to decide if it is allowed.

We are essentially saying that a behavior that was previously a user error (repeatedly obtain Tracer/Meter/Logger with the same name/version/schema_url and different attributes) is now valid.

Can we treat this as a backward compatible, non-breaking enhancement that fits our stability guarantees?

For reference, From mentioned issue description:

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.

@yurishkuro
Copy link
Member

We are essentially saying that a behavior that was previously a user error (repeatedly obtain Tracer/Meter/Logger with the same name/version/schema_url and different attributes) is now valid.

That is generally considered a backwards compatible change, because it relaxes the input requirements of the API. A step in the opposite direction (error where there wasn't one previously) would be a breaking change.

@pellared pellared changed the title Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger [POC] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Jul 25, 2024
@pellared pellared changed the title [POC] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger [POC] [DoNotMerge] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Jul 25, 2024
@pellared pellared marked this pull request as ready for review July 25, 2024 17:36
@pellared pellared requested review from a team July 25, 2024 17:36
@pellared pellared changed the title [POC] [DoNotMerge] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger [DoNotMerge] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Jul 30, 2024
@pellared
Copy link
Member Author

Status update:
In August, I am on vacation. I am perfectly fine if someone takes the issue/PR after me to keep the momentum.
In September, I plan to make a [draft] PR in OTel Go.

@pellared pellared changed the title [DoNotMerge] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Jul 31, 2024
@pellared pellared changed the title Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger [DoNotMerge] Define attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger Jul 31, 2024
cijothomas
cijothomas reviewed Sep 18, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
cijothomas
cijothomas reviewed Sep 18, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
pellared and others added 2 commits September 18, 2024 22:04
@pellared @cijothomas
Update CHANGELOG.md
c2baec9 
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
@pellared
Update CHANGELOG.md
091c853
@pellared pellared requested review from a team as code owners September 19, 2024 13:44
@pellared pellared added the area:api Cross language API specification issue label Sep 19, 2024
@dashpole
Copy link
Contributor

I think this needs updates to some of the compatibility docs (at least prometheus). Currently, Prometheus adds scope name and scope version as labels to ensure we don't end up with duplicate timeseries, and we are able to put scope attributes in a separate otel_scope_info metric. With this change, we will need to update this to instead add the scope name, scope version, and scope attributes to all metrics.

TIL schema_url is also identifying, so that is something we will need to handle in Prometheus export and other "non-OTLP" compatibility documents.

@pellared
Copy link
Member Author

pellared commented Sep 19, 2024 •
edited
Loading

I think this needs updates to some of the compatibility docs (at least prometheus). Currently, Prometheus adds scope name and scope version as labels to ensure we don't end up with duplicate timeseries, and we are able to put scope attributes in a separate otel_scope_info metric. With this change, we will need to update this to instead add the scope name, scope version, and scope attributes to all metrics.

TIL schema_url is also identifying, so that is something we will need to handle in Prometheus export and other "non-OTLP" compatibility documents.

@dashpole, are you able to help with it? Do you think that it could be done in a separate PR? Should it be blocking this PR or can it be handled afterwards?

Side note: Zipkin exporter does not export scope info at all (name, version, schemeURL, attributes).

@reyang reyang merged commit ee00d2e into open-telemetry:main Sep 24, 2024
6 checks passed
@cijothomas cijothomas mentioned this pull request Sep 24, 2024
Open
@pellared pellared mentioned this pull request Sep 24, 2024
Open
This was referenced Oct 3, 2024
Merged
Merged
carlosalberto pushed a commit that referenced this pull request Oct 9, 2024
@aabmass
Update Instrumentation Scope glossary entry with correct identifying …
e5b37e1 
…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
@iRevive iRevive mentioned this pull request Oct 11, 2024
Merged
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@pellared
Define attributes equality and make all fields as identifying for Tra…
606df78 
…cer, Meter, Logger, EventLogger (open-telemetry#4161)
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@aabmass
Update Instrumentation Scope glossary entry with correct identifying …
b1902e5 
…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
Reviewers

@MrAlias MrAlias MrAlias approved these changes

@cijothomas cijothomas cijothomas approved these changes

@jmacd jmacd jmacd approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@jsuereth jsuereth jsuereth approved these changes

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan

Assignees

@jmacd jmacd

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Define how providers handle instrumentation scope attributes and identical creation calls
10 participants
@pellared @tigrannajaryan @yurishkuro @cijothomas @dashpole @jsuereth @carlosalberto @jmacd @MrAlias @reyang