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

Audit the addition of the synchronous gauge #5369

Closed
MrAlias opened this issue May 16, 2024 · 11 comments
Closed

Audit the addition of the synchronous gauge #5369

MrAlias opened this issue May 16, 2024 · 11 comments
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented May 16, 2024

#5304

Added to the spec here

@MrAlias MrAlias added the area:metrics Part of OpenTelemetry Metrics label May 16, 2024
@MrAlias MrAlias added this to the v1.27.0 milestone May 16, 2024
@MrAlias
Copy link
Contributor Author

MrAlias commented May 17, 2024

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:

// Int64Gauge returns a new Int64Gauge instrument identified by name and
// configured with options. The instrument is used to synchronously record
// instantaneous int64 measurements during a computational operation.
Int64Gauge(name string, options ...Int64GaugeOption) (Int64Gauge, error)

// Float64Gauge returns a new Float64Gauge instrument identified by name and
// configured with options. The instrument is used to synchronously record
// instantaneous float64 measurements during a computational operation.
Float64Gauge(name string, options ...Float64GaugeOption) (Float64Gauge, error)

@MrAlias
Copy link
Contributor Author

MrAlias commented May 17, 2024

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
Copy link
Contributor Author

MrAlias commented May 17, 2024

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:

// Record records the instantaneous value.
//
// Use the WithAttributeSet (or, if performance is not a concern,
// the WithAttributes) option to include measurement attributes.
Record(ctx context.Context, value float64, options ...RecordOption)

// Record records the instantaneous value.
//
// Use the WithAttributeSet (or, if performance is not a concern,
// the WithAttributes) option to include measurement attributes.
Record(ctx context.Context, value int64, options ...RecordOption)

@MrAlias
Copy link
Contributor Author

MrAlias commented May 17, 2024

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:

// Record records the instantaneous value.
//
// Use the WithAttributeSet (or, if performance is not a concern,
// the WithAttributes) option to include measurement attributes.
Record(ctx context.Context, value float64, options ...RecordOption)

// Record records the instantaneous value.
//
// Use the WithAttributeSet (or, if performance is not a concern,
// the WithAttributes) option to include measurement attributes.
Record(ctx context.Context, value int64, options ...RecordOption)

The WithAttributeSet and WithAttributes both return a MeasurmentOption:

// WithAttributeSet sets the attribute Set associated with a measurement is
// made with.
//
// If multiple WithAttributeSet or WithAttributes options are passed the
// attributes will be merged together in the order they are passed. Attributes
// with duplicate keys will use the last value passed.
func WithAttributeSet(attributes attribute.Set) MeasurementOption {
return attrOpt{set: attributes}
}

RecordOption

These MeasurmentOptions can be used as a RecordOption:

// MeasurementOption applies options to all instrument measurement.
type MeasurementOption interface {
AddOption
RecordOption
ObserveOption
}

We comply with this requirement from the specification.

@MrAlias
Copy link
Contributor Author

MrAlias commented May 17, 2024

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
Copy link
Contributor Author

MrAlias commented May 17, 2024

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.

// Int64Gauge returns a new Int64Gauge instrument identified by name and
// configured with options. The instrument is used to synchronously record
// instantaneous int64 measurements during a computational operation.
Int64Gauge(name string, options ...Int64GaugeOption) (Int64Gauge, error)

// Float64Gauge returns a new Float64Gauge instrument identified by name and
// configured with options. The instrument is used to synchronously record
// instantaneous float64 measurements during a computational operation.
Float64Gauge(name string, options ...Float64GaugeOption) (Float64Gauge, error)

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
Copy link
Contributor Author

MrAlias commented May 17, 2024

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:

case AggregationLastValue:
switch kind {
case InstrumentKindGauge:
meas, comp = b.LastValue()

For delta temporality:

case metricdata.DeltaTemporality:
return b.filter(lv.measure), lv.delta

For cumulative temporality:

default:
return b.filter(lv.measure), lv.cumulative

We comply with this requirement.

@MrAlias
Copy link
Contributor Author

MrAlias commented May 17, 2024

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:

func (s *lastValue[N]) delta(dest *metricdata.Aggregation) int {
// Ignore if dest is not a metricdata.Gauge. The chance for memory reuse of
// the DataPoints is missed (better luck next time).
gData, _ := (*dest).(metricdata.Gauge[N])
s.Lock()
defer s.Unlock()
n := s.copyDpts(&gData.DataPoints)
// Do not report stale values.
clear(s.values)
// Update start time for delta temporality.
s.start = now()
*dest = gData
return n
}

cumulative:

func (s *lastValue[N]) cumulative(dest *metricdata.Aggregation) int {
// Ignore if dest is not a metricdata.Gauge. The chance for memory reuse of
// the DataPoints is missed (better luck next time).
gData, _ := (*dest).(metricdata.Gauge[N])
s.Lock()
defer s.Unlock()
n := s.copyDpts(&gData.DataPoints)
// TODO (#3006): This will use an unbounded amount of memory if there
// are unbounded number of attribute sets being aggregated. Attribute
// sets that become "stale" need to be forgotten so this will not
// overload the system.
*dest = gData
return n
}

With start being set on creation:

func newLastValue[N int64 | float64](limit int, r func() exemplar.Reservoir) *lastValue[N] {
return &lastValue[N]{
newRes: r,
limit: newLimiter[datapoint[N]](limit),
values: make(map[attribute.Distinct]datapoint[N]),
start: now(),
}
}

@MrAlias
Copy link
Contributor Author

MrAlias commented May 17, 2024

@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
Copy link
Contributor Author

MrAlias commented May 17, 2024

From #5369 (comment)

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 #5377

@dashpole
Copy link
Contributor

Thanks @MrAlias. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics
Projects
None yet
Development

No branches or pull requests

2 participants