Skip to content

Commit

Permalink
sdk/metric: Record measurements when context is done (#4671)
Browse files Browse the repository at this point in the history
  • Loading branch information
pellared committed Dec 15, 2023
1 parent 057f897 commit 8e75651
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 12 deletions.
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

0 comments on commit 8e75651

Please sign in to comment.