From 8e756513a630cc0e80c8b65528f27161a87a3cc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Fri, 15 Dec 2023 19:14:16 +0100 Subject: [PATCH] sdk/metric: Record measurements when context is done (#4671) --- CHANGELOG.md | 2 ++ CONTRIBUTING.md | 20 ++++++++++++++++++++ sdk/metric/instrument.go | 6 ------ sdk/metric/meter_test.go | 16 ++++++++++------ sdk/trace/trace_test.go | 12 ++++++++++++ 5 files changed, 44 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2376a26e50..3c0afb9c957 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 850606ae692..868fdf62c5f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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 diff --git a/sdk/metric/instrument.go b/sdk/metric/instrument.go index 30373038f5e..d549dc17a20 100644 --- a/sdk/metric/instrument.go +++ b/sdk/metric/instrument.go @@ -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) } @@ -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) } diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index c661a06dabb..adf3cd251e2 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -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 { @@ -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", @@ -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", @@ -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", @@ -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", @@ -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", @@ -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", diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 5ad35314192..469eb12ecba 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -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")