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 attributes equality and make all fields as identifying for Tracer, Meter, Logger, EventLogger #4161

Merged
merged 19 commits into from
Sep 24, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Jul 25, 2024

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 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)

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
@pellared pellared added the spec:trace Related to the specification/trace directory label Sep 11, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@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

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
carlosalberto pushed a commit that referenced this pull request 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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define how providers handle instrumentation scope attributes and identical creation calls
10 participants