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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ 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 measurment in such scenario then you need to handle it yourself.
You can do it e.g. by adding `if ctx.Err() != nil` condition before making the measurement.
However, we believe that you rather not want it as the telemetry may be necessary to diagnose a cancellation-related problem. (#4671)
MrAlias marked this conversation as resolved.
Show resolved Hide resolved
pellared marked this conversation as resolved.
Show resolved Hide resolved
- 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
14 changes: 14 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,20 @@ this.

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

### Honoring context cancellation
pellared marked this conversation as resolved.
Show resolved Hide resolved

The implementation of OpenTelemetry API that is responsible for telemetry recording
(starting a span, making a synchronous metric measurement, emitting a log)
must not honor the cancelation of the passed context.
pellared marked this conversation as resolved.
Show resolved Hide resolved
Among other things, the telemetry may be necessary to diagnose a
cancellation-related problem.
The context can be used to pass request-scoped values
such as Trace ID and Span ID.
pellared marked this conversation as resolved.
Show resolved Hide resolved

For other use cases
(exporting telemetry, force flushing telemetry, shutting down a signal provider)
the context cancelation should be honored.
pellared marked this conversation as resolved.
Show resolved Hide resolved

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