search

open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.29k stars 1.08k forks source link

Audit the addition of the synchronous gauge #5369

Closed MrAlias closed 6 months ago

MrAlias commented 6 months ago

https://github.com/open-telemetry/opentelemetry-go/pull/5304

Added to the spec here

MrAlias commented 6 months ago

There MUST NOT be any API for creating a Gauge other than with a Meter.

From the exported API, the only way to create a (non-trivial nil) Gauge is via the Meter API:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/meter.go#L61-L64

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/meter.go#L111-L114

MrAlias commented 6 months ago

This MAY be called CreateGauge. If strong type is desired, OpenTelemetry API authors MAY decide the language idiomatic name(s), for example CreateUInt64Gauge, CreateDoubleGauge, CreateGauge, CreateGauge.

We do not follow this optional. Our names are Int64Gauge and Float64Gauge.

Given the optional nature, we remain compliant.

MrAlias commented 6 months ago

This API SHOULD NOT return a value (it MAY return a dummy value if required by certain programming languages or systems, for example null, undefined).

Our interface type declarations require the type to not return a value:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/syncfloat64.go#L187-L191

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/syncint64.go#L187-L191

MrAlias commented 6 months ago

This API MUST accept the following parameter:

  • A numeric value. The current absolute value.

    The value needs to be provided by a user. If possible, this API SHOULD be structured so a user is obligated to provide this parameter. If it is not possible to structurally enforce this obligation, this API MUST be documented in a way to communicate to users that this parameter is needed.

  • Attributes to associate with the value.

    Users can provide attributes to associate with the value, but it is up to their discretion. Therefore, this API MUST be structured to accept a variable number of attributes, including none.

Our interface type declarations require a value parameter and can accept options:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/syncfloat64.go#L187-L191

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/syncint64.go#L187-L191

The WithAttributeSet and WithAttributes both return a MeasurmentOption:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/instrument.go#L339-L347

RecordOption

These MeasurmentOptions can be used as a RecordOption:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/instrument.go#L283-L288

We comply with this requirement from the specification.

MrAlias commented 6 months ago

The OpenTelemetry API authors MAY decide to allow flexible attributes to be passed in as arguments. If the attribute names and types are provided during the gauge creation, the OpenTelemetry API authors MAY allow attribute values to be passed in using a more efficient way (e.g. strong typed struct allocated on the callstack, tuple). The API MUST allow callers to provide flexible attributes at invocation time rather than having to register all the possible attribute names during the instrument creation.

We do not provide the optional ability to accept 'flexible attributes' during the gauge creation. We do comply with the requirement to accept attributes at "invocation time" as outlined here.

MrAlias commented 6 months ago

See the general requirements for synchronous instruments.

https://opentelemetry.io/docs/specs/otel/metrics/api/#synchronous-instrument-api

The API to construct synchronous instruments MUST accept the following parameters:

  • A name of the Instrument.

    The name needs to be provided by a user. If possible, the API SHOULD be structured so a user is obligated to provide this parameter. If it is not possible to structurally enforce this obligation, the API MUST be documented in a way to communicate to users that this parameter is needed. The API SHOULD be documented in a way to communicate to users that the name parameter needs to conform to the instrument name syntax. The API SHOULD NOT validate the name; that is left to implementations of the API.

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/meter.go#L61-L64

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/metric/meter.go#L111-L114

We comply with the required structure of accepting a name.

The recommendation to document what the name should be is not present. Nor is is present on any other method.

We need to track this as a fix for all methods. It is not blocking for this issue.

We do not validate the name in the API.

  • A unit of measure.

    Users can provide a unit, but it is up to their discretion. Therefore, this API needs to be structured to accept a unit, but MUST NOT obligate a user to provide one.

    The unit parameter needs to support the instrument unit rule. Meaning, the API MUST accept a case-sensitive string that supports ASCII character encoding and can hold at least 63 characters. The API SHOULD NOT validate the unit.

WithUnit can be used to pass an InstrumentOption. The user is not obligated to do this.

We do not validate the unit.

  • A description describing the Instrument in human-readable terms.

    Users can provide a description, but it is up to their discretion. Therefore, this API needs to be structured to accept a description, but MUST NOT obligate a user to provide one.

    The description needs to support the instrument description rule. Meaning, the API MUST accept a string that supports at least BMP (Unicode Plane 0) encoded characters and hold at least 1023 characters.

WithDescription can be used to pass an InstrumentOption. The user is not obligated to do this.

  • advisory parameters associated with the instrument kind.

    Users can provide advisory parameters, but its up to their discretion. Therefore, this API needs to be structured to accept advisory parameters, but MUST NOT obligate the user to provide it.

    advisory parameters need to be structured as described in instrument advisory parameters, with parameters that are general and specific to a particular instrument kind. The API SHOULD NOT validate advisory parameters.

There are no stable advisory parameters that apply to this instrument kind: https://opentelemetry.io/docs/specs/otel/metrics/api/#instrument-advisory-parameters

When applicable advisory parameters are added or stabilized they can be added as options.

MrAlias commented 6 months ago

For synchronous instruments with Cumulative aggregation temporality, MetricReader.Collect MUST receive data points exposed in previous collections regardless of whether new measurements have been recorded. For synchronous instruments with Delta aggregation temporality, MetricReader.Collect MUST only receive data points with measurements recorded since the previous collection.

The aggregation selection in the SDK:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/sdk/metric/pipeline.go#L449-L452

For delta temporality:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/sdk/metric/internal/aggregate/aggregate.go#L80-L81

For cumulative temporality:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/sdk/metric/internal/aggregate/aggregate.go#L82-L83

We comply with this requirement.

MrAlias commented 6 months ago

The MetricReader selection of temporality as a function of instrument kind influences the starting timestamp (i.e. StartTimeUnixNano) of metrics data points received by MetricReader.Collect. For instruments with Cumulative aggregation temporality, successive data points received by successive calls to MetricReader.Collect MUST repeat the same starting timestamps (e.g. (T0, T1], (T0, T2], (T0, T3]). For instruments with Delta aggregation temporality, successive data points received by successive calls to MetricReader.Collect MUST advance the starting timestamp ( e.g. (T0, T1], (T1, T2], (T2, T3]). The ending timestamp (i.e. TimeUnixNano) MUST always be equal to time the metric data point took effect, which is equal to when MetricReader.Collect was invoked. These rules apply to all metrics, not just those whose point kinds includes an aggregation temporality field.

delta:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/sdk/metric/internal/aggregate/lastvalue.go#L63-L80

cumulative:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/sdk/metric/internal/aggregate/lastvalue.go#L82-L98

With start being set on creation:

https://github.com/open-telemetry/opentelemetry-go/blob/ebd0adee357f3539d7b19488eef9cf7bcd8aa0da/sdk/metric/internal/aggregate/lastvalue.go#L24-L31

MrAlias commented 6 months ago

@dashpole (and all other @open-telemetry/go-approvers) I believe I've completed the audit. If you concur, please go ahead and close this issue.

MrAlias commented 6 months ago

From https://github.com/open-telemetry/opentelemetry-go/issues/5369#issuecomment-2118274205

The recommendation to document what the name should be is not present. Nor is is present on any other method.

We need to track this as a fix for all methods. It is not blocking for this issue.

Tracking in https://github.com/open-telemetry/opentelemetry-go/issues/5377

dashpole commented 6 months ago

Thanks @MrAlias. LGTM