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

Add support for instrumentation scope attributes during meter/tracer/logger creation #3368

Closed
paivagustavo opened this issue Oct 19, 2022 · 13 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@paivagustavo
Copy link
Member

Problem Statement

Currently, there is no way to define instrumentation scope attributes when creating a tracer/meter as specified in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter

Proposed Solution

Add back instrumentation scope attributes.

Prior Art

This was already implemented by #3132 but it was reverted because specs was not ready yet (#3154).

@paivagustavo paivagustavo added the enhancement New feature or request label Oct 19, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Oct 19, 2022

Similar to #3140, this cannot be added until we have a clear specification for how to export these values. Specifically how conflicts are resolved given these attributes are not identifying.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 16, 2023

Both #3739 and #3738 add options to the trace and metric API to set the instrumentation scope attributes. The SDKs in both situation do not do anything with this information.

@pellared
Copy link
Member

pellared commented Jan 31, 2024

I think that

  1. we should add a Attributes field to instrumentation.Scope
  2. the SDK should honor set these fields when WithAttributes options are passed to Meter (or Tracer)
  3. the OTLP exporter should honor these values and pass it via https://github.com/open-telemetry/opentelemetry-proto/blob/b5c1a7882180a26bb7794594e8546798ecb68103/opentelemetry/proto/common/v1/common.proto#L76-L79

@MrAlias Do you still see these as being blocked by the specification? Is there any existing issue in the specification? Do you have some recommendation on how to proceed/unblock (e.g. where we should define such information)?

@MrAlias
Copy link
Contributor

MrAlias commented Jan 31, 2024

@pellared how will we distinguish between instruments scopes if attributes are added. They are a non-identifying field, they cannot be used in comparisons.

How will we resolve duplicate instrument scopes being passed with different attributes. The specification does not say how this is to be resolved. If we make a decision here it will be unspecified and likely to conflict with what the specification ultimately decides.

@pellared
Copy link
Member

pellared commented Jan 31, 2024

@pellared how will we distinguish between instruments scopes if attributes are added. They are a non-identifying field, they cannot be used in comparisons.

From spec

Tracers are identified by name, version, and schema_url fields. 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. It is a user error to create Tracers with different attributes but the same identity.

Meters are identified by name, version, and schema_url fields. When more than one Meter of the same name, version, and schema_url is created, it is unspecified whether or under which conditions the same or different Meter instances are returned. It is a user error to create Meters with different attributes but the same identity.

Loggers are identified by name, version, and schema_url fields. When more than one Logger of the same name, version, and schema_url is created, it is unspecified whether or under which conditions the same or different Logger instances are returned. It is a user error to create Loggers with different attributes but the same identity.

I think we could simply create a type which is like instrumentation.Scope but without Attributes so that use it in maps. We can introduce types like meterID, tracerID with fields that are sued to identify a meter or logger.

How will we resolve duplicate instrument scopes being passed with different attributes. The specification does not say how this is to be resolved. If we make a decision here it will be unspecified and likely to conflict with what the specification ultimately decides.

Trying to solve this here: open-telemetry/opentelemetry-specification#3855

@MrAlias
Copy link
Contributor

MrAlias commented Jan 31, 2024

How will we resolve duplicate instrument scopes being passed with different attributes. The specification does not say how this is to be resolved. If we make a decision here it will be unspecified and likely to conflict with what the specification ultimately decides.

Trying to solve this here: open-telemetry/opentelemetry-specification#3855

How does that resolve the questions quoted? It is adjusting definitions to include logs in instrumentation scope references. The questions already exist for all signal types that use instrument scope.

@MrAlias
Copy link
Contributor

MrAlias commented Jan 31, 2024

I think we could simply create a type which is like instrumentation.Scope but without Attributes so that use it in maps. We can introduce types like meterID, tracerID with fields that are sued to identify a meter or logger.

Where will meterID and tracerID live? Will this cause cross package internal use?

@pellared
Copy link
Member

pellared commented Jan 31, 2024

How does that resolve the questions quoted? It is adjusting definitions to include logs in instrumentation scope references. The questions already exist for all signal types that use instrument scope.

Sorry, my PR was only about how to solve scope attribute with duplicate keys 🤦

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

Therefore, I think we should do exactly the same as we do right now when the tracer/logger that have "the same IDs" (return the existing tracer that we have in the map).

@MrAlias
Copy link
Contributor

MrAlias commented Jan 31, 2024

Therefore, I think we should do exactly the same as we do right now when the tracer/logger that have "the same IDs" (return the existing tracer that we have in the map).

So drop the second set of attributes. Again, I go back to the lack of definition in the specification. That behavior is not defined there. There are "TODO"s in the spec to define the behavior, but there is not stable specification defining it.

I do not support implementing a behavior in our implementation that is specifically called out in the specification as something they will decide in the future. Especially since it may ultimately be a different behavior. The last time this was brought up in the SIG meeting this was the SIG consensus as well.

@pellared
Copy link
Member

Thanks. I will try to refine the specification as part of my work for OTel Go Logs.

@pellared pellared self-assigned this Jan 31, 2024
@pellared pellared changed the title Add support for instrumentation scope attributes during meter and tracer creation Add support for instrumentation scope attributes during meter/tracer/logger creation Jul 15, 2024
@pellared
Copy link
Member

Thanks. I will try to refine the specification as part of my work for OTel Go Logs.

I started by creating open-telemetry/opentelemetry-specification#4146

@pellared
Copy link
Member

open-telemetry/opentelemetry-specification#4160 is merged.
Unblocked from the specification perspective. I will start working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Closed
Status: Done
Development

No branches or pull requests

3 participants