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

sdk/metric: Record measurements when context is done #4671

Merged
merged 15 commits into from
Dec 15, 2023
Merged
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Changed

- Record synchronous measurements when the passed context is canceled instead of dropping in `go.opentelemetry.io/otel/sdk/metric`.
If you do not want to make a measurement when the context is cancelled, you need to handle it yourself (e.g `if ctx.Err() != nil`). (#4671)
- Improve `go.opentelemetry.io/otel/trace.TraceState`'s performance. (#4722)
- Improve `go.opentelemetry.io/otel/propagation.TraceContext`'s performance. (#4721)
- Improve `go.opentelemetry.io/otel/baggage` performance. (#4743)
Expand Down
20 changes: 20 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,26 @@ this.

[^3]: https://github.com/open-telemetry/opentelemetry-go/issues/3548

### Ignoring context cancellation

OpenTelemetry API implementations need to ignore the cancellation of the context that are
passed when recording a value (e.g. starting a span, recording a measurement, emitting a log).
Recording methods should not return an error describing the cancellation state of the context
when they complete, nor should they abort any work.

This rule may not apply if the OpenTelemetry specification defines a timeout mechanism for
the method. In that case the context cancellation can be used for the timeout with the
restriction that this behavior is documented for the method. Otherwise, timeouts
are expected to be handled by the user calling the API, not the implementation.

Stoppage of the telemetry pipeline is handled by calling the appropriate `Shutdown` method
of a provider. It is assumed the context passed from a user is not used for this purpose.

Outside of the direct recording of telemetry from the API (e.g. exporting telemetry,
force flushing telemetry, shutting down a signal provider) the context cancellation
should be honored. This means all work done on behalf of the user provided context
should be canceled.

## Approvers and Maintainers

### Approvers
Expand Down
6 changes: 0 additions & 6 deletions sdk/metric/instrument.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,6 @@ func (i *int64Inst) Record(ctx context.Context, val int64, opts ...metric.Record
}

func (i *int64Inst) aggregate(ctx context.Context, val int64, s attribute.Set) { // nolint:revive // okay to shadow pkg with method.
if err := ctx.Err(); err != nil {
return
}
for _, in := range i.measures {
in(ctx, val, s)
}
Expand Down Expand Up @@ -238,9 +235,6 @@ func (i *float64Inst) Record(ctx context.Context, val float64, opts ...metric.Re
}

func (i *float64Inst) aggregate(ctx context.Context, val float64, s attribute.Set) {
if err := ctx.Err(); err != nil {
return
}
for _, in := range i.measures {
in(ctx, val, s)
}
Expand Down
16 changes: 10 additions & 6 deletions sdk/metric/meter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,10 @@ func TestCallbackUnregisterConcurrency(t *testing.T) {

// Instruments should produce correct ResourceMetrics.
func TestMeterCreatesInstruments(t *testing.T) {
// The synchronous measurement methods must ignore the context cancelation.
ctx, cancel := context.WithCancel(context.Background())
cancel()

attrs := attribute.NewSet(attribute.String("name", "alice"))
opt := metric.WithAttributeSet(attrs)
testCases := []struct {
Expand Down Expand Up @@ -340,7 +344,7 @@ func TestMeterCreatesInstruments(t *testing.T) {
ctr, err := m.Int64Counter("sint")
assert.NoError(t, err)

ctr.Add(context.Background(), 3)
ctr.Add(ctx, 3)
},
want: metricdata.Metrics{
Name: "sint",
Expand All @@ -359,7 +363,7 @@ func TestMeterCreatesInstruments(t *testing.T) {
ctr, err := m.Int64UpDownCounter("sint")
assert.NoError(t, err)

ctr.Add(context.Background(), 11)
ctr.Add(ctx, 11)
},
want: metricdata.Metrics{
Name: "sint",
Expand All @@ -378,7 +382,7 @@ func TestMeterCreatesInstruments(t *testing.T) {
gauge, err := m.Int64Histogram("histogram")
assert.NoError(t, err)

gauge.Record(context.Background(), 7)
gauge.Record(ctx, 7)
},
want: metricdata.Metrics{
Name: "histogram",
Expand All @@ -404,7 +408,7 @@ func TestMeterCreatesInstruments(t *testing.T) {
ctr, err := m.Float64Counter("sfloat")
assert.NoError(t, err)

ctr.Add(context.Background(), 3)
ctr.Add(ctx, 3)
},
want: metricdata.Metrics{
Name: "sfloat",
Expand All @@ -423,7 +427,7 @@ func TestMeterCreatesInstruments(t *testing.T) {
ctr, err := m.Float64UpDownCounter("sfloat")
assert.NoError(t, err)

ctr.Add(context.Background(), 11)
ctr.Add(ctx, 11)
},
want: metricdata.Metrics{
Name: "sfloat",
Expand All @@ -442,7 +446,7 @@ func TestMeterCreatesInstruments(t *testing.T) {
gauge, err := m.Float64Histogram("histogram")
assert.NoError(t, err)

gauge.Record(context.Background(), 7)
gauge.Record(ctx, 7)
},
want: metricdata.Metrics{
Name: "histogram",
Expand Down
12 changes: 12 additions & 0 deletions sdk/trace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1133,6 +1133,18 @@ func TestNilSpanEnd(t *testing.T) {
span.End()
}

func TestSpanWithCanceledContext(t *testing.T) {
te := NewTestExporter()
tp := NewTracerProvider(WithSyncer(te))

ctx, cancel := context.WithCancel(context.Background())
cancel()
_, span := tp.Tracer(t.Name()).Start(ctx, "span")
span.End()

assert.Equal(t, 1, te.Len(), "span recording must ignore context cancelation")
}

func TestNonRecordingSpanDoesNotTrackRuntimeTracerTask(t *testing.T) {
tp := NewTracerProvider(WithSampler(NeverSample()))
tr := tp.Tracer("TestNonRecordingSpanDoesNotTrackRuntimeTracerTask")
Expand Down