opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

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

Open paivagustavo opened this issue 3 years ago • 13 comments

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 https://github.com/open-telemetry/opentelemetry-go/pull/3132 but it was reverted because specs was not ready yet (https://github.com/open-telemetry/opentelemetry-go/pull/3154).

paivagustavo avatar Oct 19 '22 21:10 paivagustavo

Similar to https://github.com/open-telemetry/opentelemetry-go/issues/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 avatar Oct 19 '22 21:10 MrAlias

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.

MrAlias avatar Feb 16 '23 21:02 MrAlias

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

pellared avatar Jan 31 '24 12:01 pellared

@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.

MrAlias avatar Jan 31 '24 16:01 MrAlias

@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: https://github.com/open-telemetry/opentelemetry-specification/pull/3855

pellared avatar Jan 31 '24 19:01 pellared

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 avatar Jan 31 '24 19:01 MrAlias

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?

MrAlias avatar Jan 31 '24 19:01 MrAlias

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

pellared avatar Jan 31 '24 19:01 pellared

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.

MrAlias avatar Jan 31 '24 19:01 MrAlias

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

pellared avatar Jan 31 '24 19:01 pellared

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

I started by creating https://github.com/open-telemetry/opentelemetry-specification/pull/4146

pellared avatar Jul 15 '24 20:07 pellared