Verify compliant metric SDK specification implementation: MeterProvider/Meter Creation

[MrAlias]

• Identify all the normative requirements, recommendations, and options the specification defines as comments to this issue
• Ensure the current metric SDK implementation is compliant with these normative requirements, recommendations, and options in those comments.

[pellared]

New Meter instances are always created through a MeterProvider (see API). The name, version (optional), schema_url (optional), and attributes (optional) arguments supplied to the MeterProvider MUST be used to create an InstrumentationScope instance which is stored on the created Meter.

The SDK's metric.NewMeterProvider returns an object which implements API's MeterProvider interface and has the Meter method which accepts a mandatory name and optional variadic MeterOption arguments (for version, schema_url, attributes).

[pellared]

In the case where an invalid name (null or empty string) is specified, a working Meter MUST be returned as a fallback rather than returning null or throwing an exception, its name SHOULD keep the original invalid value, and a message reporting that the specified value is invalid SHOULD be logged.

In the case where an invalid name (null or empty string) is specified, a working Meter MUST be returned as a fallback rather than returning null or throwing an exception, its name SHOULD keep the original invalid value,

This is implemented, the name is passed without any modification: https://github.com/open-telemetry/opentelemetry-go/blob/sdk/metric/v0.39.0/sdk/metric/provider.go#L70-L80

However, the docs are lying: https://github.com/open-telemetry/opentelemetry-go/blob/sdk/metric/v0.39.0/sdk/metric/provider.go#L63-L64

Issue created:

• Fix Meter method Go doc #4150

a message reporting that the specified value is invalid SHOULD be logged

This does not look to be implemented.

See: https://github.com/open-telemetry/opentelemetry-go/blob/sdk/metric/v0.39.0/sdk/metric/provider.go#L70-L80

Issue created:

• Log that meter name is invalid #4151

[pellared]

When a Schema URL is passed as an argument when creating a Meter the emitted telemetry for that Meter MUST be associated with the Schema URL, provided that the emitted data format is capable of representing such association.

This is implemented and tested here: https://github.com/open-telemetry/opentelemetry-go/blob/v1.16.0/metric/config_test.go#L26

[pellared]

Configuration (i.e., MetricExporters, MetricReaders and Views) MUST be managed solely by the MeterProvider and the SDK MUST provide a way to configure all options that are implemented by the SDK. This MAY be done at the time of MeterProvider creation if appropriate.

I read it three times and I am not sure what the author had in mind. Origins from: open-telemetry/opentelemetry-specification#1730

What does "configuration" mean here? What does "manage" mean? Does it mean that we cannot configure a PeriodicReader using options?

What does "SDK MUST provide a way to configure all options that are implemented by the SDK" mean (it looks like "the SDK has everything configurable").

Maybe it is my reading, but I find this paragraph not clear. Moreover, the paragraph contains normative wording.

@MrAlias Can you please double-check? (Maybe the issue is on my side)

EDIT: I guess that what the author had in mind is that the MetricProvider takes the responsibility of instantiating components (readers, exporter, views) which do actual work. If my guess is correct this would mean that if the user sets up a periodic reader it should be "started" by the metric provider. If so then e.g. this would violate the specification: https://github.com/open-telemetry/opentelemetry-go/blob/sdk/metric/v0.39.0/sdk/metric/periodic_reader.go#L130-L133.

[pellared]

The MeterProvider MAY provide methods to update the configuration. If configuration is updated (e.g., adding a MetricReader), the updated configuration MUST also apply to all already returned Meters (i.e. it MUST NOT matter whether a Meter was obtained from the MeterProvider before or after the configuration change).
Note: Implementation-wise, this could mean that Meter instances have a reference to their MeterProvider and access configuration only via this reference.

This is optional and it is not implemented.

[MrAlias]

Configuration (i.e., MetricExporters, MetricReaders and Views) MUST be managed solely by the MeterProvider and the SDK MUST provide a way to configure all options that are implemented by the SDK. This MAY be done at the time of MeterProvider creation if appropriate.

I read it three times and I am not sure what the author had in mind. Origins from: open-telemetry/opentelemetry-specification#1730

What does "configuration" mean here? What does "manage" mean? Does it mean that we cannot configure a PeriodicReader using options?

What does "SDK MUST provide a way to configure all options that are implemented by the SDK" mean (it looks like "the SDK has everything configurable").

Maybe it is my reading, but I find this paragraph not clear. Moreover, the paragraph contains normative wording.

@MrAlias Can you please double-check? (Maybe the issue is on my side)

This looks similar to the trace SDK where the TracerProvider held all the configuration (if I remember correctly it was just copied from the trace specification). The intent, as I understand it, is that there shouldn't be some other object other than the MeterProvider that holds configuration, like a Configurator object.

This does seem strange though given the exporter and reader will configure the temporality and default aggregators.

Given our setup matches other already stable language implementations, I'm guessing we aren't doing something the specification didn't intend.

I would suggest opening a PR in the specification to clarify this section after working with the spec SIG to understand its meaning. That way if we are out of compliance we can fix it now.

[MrAlias]

EDIT: I guess that what the author had in mind is that the MetricProvider takes the responsibility of instantiating components (readers, exporter, views) which do actual work. If my guess is correct this would mean that if the user sets up a periodic reader it should be "started" by the metric provider. If so then e.g. this would violate the specification: https://github.com/open-telemetry/opentelemetry-go/blob/sdk/metric/v0.39.0/sdk/metric/periodic_reader.go#L130-L133.

I don't think the started nature matters too much here. More so the configuration of how a reader fits into the metric pipelines.

[pellared]

PR created for clarifying the specification about the configuration:

• Refine SDK MeterProvider configuration section opentelemetry-specification#3522

[pellared]

open-telemetry/opentelemetry-specification#3522 is merged. Closing the issue as all related issues and PRs are closed.