From 42863522e50aa5c3a88043d1b068ccec083264a8 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 12 Jan 2023 16:01:51 -0800 Subject: [PATCH 1/2] Update ClientRequest HTTPS determination (#3577) * Update ClientRequest HTTPS determination The ClientRequest function will only report a peer port attribute if that peer port differs from the standard 80 for HTTP and 443 for HTTPS. In determining if the request is for HTTPS use the request URL scheme. This is not perfect. If a user doesn't provide a scheme this will not be correctly detected. However, the current approach of checking if the `TLS` field is non-nil will always be wrong, requests made by client ignore this field and it is always nil. Therefore, switching to using the URL field is the best we can do without having already made the request. * Test HTTPS detection for ClientRequest --- semconv/internal/v2/http.go | 2 +- semconv/internal/v2/http_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/semconv/internal/v2/http.go b/semconv/internal/v2/http.go index 6989391cfbd..0f84a905b04 100644 --- a/semconv/internal/v2/http.go +++ b/semconv/internal/v2/http.go @@ -84,7 +84,7 @@ func (c *HTTPConv) ClientRequest(req *http.Request) []attribute.KeyValue { h = req.URL.Host } peer, p := firstHostPort(h, req.Header.Get("Host")) - port := requiredHTTPPort(req.TLS != nil, p) + port := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", p) if port > 0 { n++ } diff --git a/semconv/internal/v2/http_test.go b/semconv/internal/v2/http_test.go index edfc7b50f27..73b9aa9b147 100644 --- a/semconv/internal/v2/http_test.go +++ b/semconv/internal/v2/http_test.go @@ -59,6 +59,31 @@ func TestHTTPClientResponse(t *testing.T) { }, got) } +func TestHTTPSClientRequest(t *testing.T) { + req := &http.Request{ + Method: http.MethodGet, + URL: &url.URL{ + Scheme: "https", + Host: "127.0.0.1:443", + Path: "/resource", + }, + Proto: "HTTP/1.0", + ProtoMajor: 1, + ProtoMinor: 0, + } + + assert.Equal( + t, + []attribute.KeyValue{ + attribute.String("http.method", "GET"), + attribute.String("http.flavor", "1.0"), + attribute.String("http.url", "https://127.0.0.1:443/resource"), + attribute.String("net.peer.name", "127.0.0.1"), + }, + hc.ClientRequest(req), + ) +} + func TestHTTPClientRequest(t *testing.T) { const ( user = "alice" From f941b3a8dfbca737c5698f645904abc81a95ecb7 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 13 Jan 2023 08:31:14 -0800 Subject: [PATCH 2/2] Restructure RegisterCallback method (#3587) * Restructure RegisterCallback method Instead of accepting instruments to register the callback with as a slice, accept them as variadic arguments. * Add changes to changelog * Add PR number to changes --- CHANGELOG.md | 2 + example/prometheus/main.go | 4 +- metric/example_test.go | 7 +- metric/internal/global/meter.go | 6 +- metric/internal/global/meter_test.go | 16 ++-- metric/internal/global/meter_types_test.go | 2 +- metric/meter.go | 2 +- metric/noop.go | 2 +- sdk/metric/meter.go | 2 +- sdk/metric/meter_test.go | 90 +++++++++++----------- 10 files changed, 67 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bc79e6543d..e763a5c706c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -100,6 +100,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The exporter from `go.opentelemetry.io/otel/exporters/zipkin` is updated to use the `v1.16.0` version of semantic conventions. This means it no longer uses the removed `net.peer.ip` or `http.host` attributes to determine the remote endpoint. Instead it uses the `net.sock.peer` attributes. (#3581) +- The parameters for the `RegisterCallback` method of the `Meter` from `go.opentelemetry.io/otel/metric` are changed. + The slice of `instrument.Asynchronous` parameter is now passed as a variadic argument. (#3587) ### Deprecated diff --git a/example/prometheus/main.go b/example/prometheus/main.go index 68be2f8d6cb..4e9be3b84da 100644 --- a/example/prometheus/main.go +++ b/example/prometheus/main.go @@ -68,11 +68,11 @@ func main() { if err != nil { log.Fatal(err) } - _, err = meter.RegisterCallback([]instrument.Asynchronous{gauge}, func(ctx context.Context) error { + _, err = meter.RegisterCallback(func(ctx context.Context) error { n := -10. + rand.Float64()*(90.) // [-10, 100) gauge.Observe(ctx, n, attrs...) return nil - }) + }, gauge) if err != nil { log.Fatal(err) } diff --git a/metric/example_test.go b/metric/example_test.go index 64441c23ace..4653d612e8c 100644 --- a/metric/example_test.go +++ b/metric/example_test.go @@ -89,10 +89,7 @@ func ExampleMeter_asynchronous_multiple() { gcCount, _ := meter.Int64ObservableCounter("gcCount") gcPause, _ := meter.Float64Histogram("gcPause") - _, err := meter.RegisterCallback([]instrument.Asynchronous{ - heapAlloc, - gcCount, - }, + _, err := meter.RegisterCallback( func(ctx context.Context) error { memStats := &runtime.MemStats{} // This call does work @@ -105,6 +102,8 @@ func ExampleMeter_asynchronous_multiple() { computeGCPauses(ctx, gcPause, memStats.PauseNs[:]) return nil }, + heapAlloc, + gcCount, ) if err != nil { diff --git a/metric/internal/global/meter.go b/metric/internal/global/meter.go index 97ad2735bea..92f35e9730b 100644 --- a/metric/internal/global/meter.go +++ b/metric/internal/global/meter.go @@ -278,10 +278,10 @@ func (m *meter) Float64ObservableGauge(name string, options ...instrument.Float6 // // It is only valid to call Observe within the scope of the passed function, // and only on the instruments that were registered with this call. -func (m *meter) RegisterCallback(insts []instrument.Asynchronous, f metric.Callback) (metric.Registration, error) { +func (m *meter) RegisterCallback(f metric.Callback, insts ...instrument.Asynchronous) (metric.Registration, error) { if del, ok := m.delegate.Load().(metric.Meter); ok { insts = unwrapInstruments(insts) - return del.RegisterCallback(insts, f) + return del.RegisterCallback(f, insts...) } m.mtx.Lock() @@ -335,7 +335,7 @@ func (c *registration) setDelegate(m metric.Meter) { return } - reg, err := m.RegisterCallback(insts, c.function) + reg, err := m.RegisterCallback(c.function, insts...) if err != nil { otel.Handle(err) } diff --git a/metric/internal/global/meter_test.go b/metric/internal/global/meter_test.go index 0260a7dedb3..eeb43689b0e 100644 --- a/metric/internal/global/meter_test.go +++ b/metric/internal/global/meter_test.go @@ -66,7 +66,7 @@ func TestMeterRace(t *testing.T) { _, _ = mtr.Int64Counter(name) _, _ = mtr.Int64UpDownCounter(name) _, _ = mtr.Int64Histogram(name) - _, _ = mtr.RegisterCallback(nil, func(ctx context.Context) error { return nil }) + _, _ = mtr.RegisterCallback(func(ctx context.Context) error { return nil }) if !once { wg.Done() once = true @@ -86,7 +86,7 @@ func TestMeterRace(t *testing.T) { func TestUnregisterRace(t *testing.T) { mtr := &meter{} - reg, err := mtr.RegisterCallback(nil, func(ctx context.Context) error { return nil }) + reg, err := mtr.RegisterCallback(func(ctx context.Context) error { return nil }) require.NoError(t, err) wg := &sync.WaitGroup{} @@ -128,10 +128,10 @@ func testSetupAllInstrumentTypes(t *testing.T, m metric.Meter) (instrument.Float _, err = m.Int64ObservableGauge("test_Async_Gauge") assert.NoError(t, err) - _, err = m.RegisterCallback([]instrument.Asynchronous{afcounter}, func(ctx context.Context) error { + _, err = m.RegisterCallback(func(ctx context.Context) error { afcounter.Observe(ctx, 3) return nil - }) + }, afcounter) require.NoError(t, err) sfcounter, err := m.Float64Counter("test_Async_Counter") @@ -323,10 +323,10 @@ func TestRegistrationDelegation(t *testing.T) { require.NoError(t, err) var called0 bool - reg0, err := m.RegisterCallback([]instrument.Asynchronous{actr}, func(context.Context) error { + reg0, err := m.RegisterCallback(func(context.Context) error { called0 = true return nil - }) + }, actr) require.NoError(t, err) require.Equal(t, 1, mImpl.registry.Len(), "callback not registered") // This means reg0 should not be delegated. @@ -334,10 +334,10 @@ func TestRegistrationDelegation(t *testing.T) { assert.Equal(t, 0, mImpl.registry.Len(), "callback not unregistered") var called1 bool - reg1, err := m.RegisterCallback([]instrument.Asynchronous{actr}, func(context.Context) error { + reg1, err := m.RegisterCallback(func(context.Context) error { called1 = true return nil - }) + }, actr) require.NoError(t, err) require.Equal(t, 1, mImpl.registry.Len(), "second callback not registered") diff --git a/metric/internal/global/meter_types_test.go b/metric/internal/global/meter_types_test.go index 57711f29e62..3dfc74af7b3 100644 --- a/metric/internal/global/meter_types_test.go +++ b/metric/internal/global/meter_types_test.go @@ -115,7 +115,7 @@ func (m *testMeter) Float64ObservableGauge(name string, options ...instrument.Fl // // It is only valid to call Observe within the scope of the passed function, // and only on the instruments that were registered with this call. -func (m *testMeter) RegisterCallback(i []instrument.Asynchronous, f metric.Callback) (metric.Registration, error) { +func (m *testMeter) RegisterCallback(f metric.Callback, i ...instrument.Asynchronous) (metric.Registration, error) { m.callbacks = append(m.callbacks, f) return testReg{ f: func(idx int) func() { diff --git a/metric/meter.go b/metric/meter.go index 41cbf91f255..d384d0df17e 100644 --- a/metric/meter.go +++ b/metric/meter.go @@ -102,7 +102,7 @@ type Meter interface { // // If no instruments are passed, f should not be registered nor called // during collection. - RegisterCallback(instruments []instrument.Asynchronous, f Callback) (Registration, error) + RegisterCallback(f Callback, instruments ...instrument.Asynchronous) (Registration, error) } // Callback is a function registered with a Meter that makes observations for diff --git a/metric/noop.go b/metric/noop.go index 98652b4fac7..409268ecc98 100644 --- a/metric/noop.go +++ b/metric/noop.go @@ -88,7 +88,7 @@ func (noopMeter) Float64ObservableGauge(string, ...instrument.Float64ObserverOpt } // RegisterCallback creates a register callback that does not record any metrics. -func (noopMeter) RegisterCallback([]instrument.Asynchronous, Callback) (Registration, error) { +func (noopMeter) RegisterCallback(Callback, ...instrument.Asynchronous) (Registration, error) { return noopReg{}, nil } diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index 7aabb2a999e..ab0128c98b7 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -200,7 +200,7 @@ func (m *meter) Float64ObservableGauge(name string, options ...instrument.Float6 // RegisterCallback registers the function f to be called when any of the // insts Collect method is called. -func (m *meter) RegisterCallback(insts []instrument.Asynchronous, f metric.Callback) (metric.Registration, error) { +func (m *meter) RegisterCallback(f metric.Callback, insts ...instrument.Asynchronous) (metric.Registration, error) { for _, inst := range insts { // Only register if at least one instrument has a non-drop aggregation. // Otherwise, calling f during collection will be wasted computation. diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 44209078fa6..0ea3614d03b 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -101,11 +101,11 @@ func TestMeterCallbackCreationConcurrency(t *testing.T) { m := NewMeterProvider().Meter("callback-concurrency") go func() { - _, _ = m.RegisterCallback([]instrument.Asynchronous{}, emptyCallback) + _, _ = m.RegisterCallback(emptyCallback) wg.Done() }() go func() { - _, _ = m.RegisterCallback([]instrument.Asynchronous{}, emptyCallback) + _, _ = m.RegisterCallback(emptyCallback) wg.Done() }() wg.Wait() @@ -113,7 +113,7 @@ func TestMeterCallbackCreationConcurrency(t *testing.T) { func TestNoopCallbackUnregisterConcurrency(t *testing.T) { m := NewMeterProvider().Meter("noop-unregister-concurrency") - reg, err := m.RegisterCallback(nil, emptyCallback) + reg, err := m.RegisterCallback(emptyCallback) require.NoError(t, err) wg := &sync.WaitGroup{} @@ -140,12 +140,10 @@ func TestCallbackUnregisterConcurrency(t *testing.T) { ag, err := meter.Int64ObservableGauge("gauge") require.NoError(t, err) - i := []instrument.Asynchronous{actr} - regCtr, err := meter.RegisterCallback(i, emptyCallback) + regCtr, err := meter.RegisterCallback(emptyCallback, actr) require.NoError(t, err) - i = []instrument.Asynchronous{ag} - regG, err := meter.RegisterCallback(i, emptyCallback) + regG, err := meter.RegisterCallback(emptyCallback, ag) require.NoError(t, err) wg := &sync.WaitGroup{} @@ -181,10 +179,10 @@ func TestMeterCreatesInstruments(t *testing.T) { } ctr, err := m.Int64ObservableCounter("aint", instrument.WithInt64Callback(cback)) assert.NoError(t, err) - _, err = m.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = m.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 3) return nil - }) + }, ctr) assert.NoError(t, err) // Observed outside of a callback, it should be ignored. @@ -211,10 +209,10 @@ func TestMeterCreatesInstruments(t *testing.T) { } ctr, err := m.Int64ObservableUpDownCounter("aint", instrument.WithInt64Callback(cback)) assert.NoError(t, err) - _, err = m.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = m.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 11) return nil - }) + }, ctr) assert.NoError(t, err) // Observed outside of a callback, it should be ignored. @@ -241,10 +239,10 @@ func TestMeterCreatesInstruments(t *testing.T) { } gauge, err := m.Int64ObservableGauge("agauge", instrument.WithInt64Callback(cback)) assert.NoError(t, err) - _, err = m.RegisterCallback([]instrument.Asynchronous{gauge}, func(ctx context.Context) error { + _, err = m.RegisterCallback(func(ctx context.Context) error { gauge.Observe(ctx, 11) return nil - }) + }, gauge) assert.NoError(t, err) // Observed outside of a callback, it should be ignored. @@ -269,10 +267,10 @@ func TestMeterCreatesInstruments(t *testing.T) { } ctr, err := m.Float64ObservableCounter("afloat", instrument.WithFloat64Callback(cback)) assert.NoError(t, err) - _, err = m.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = m.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 3) return nil - }) + }, ctr) assert.NoError(t, err) // Observed outside of a callback, it should be ignored. @@ -299,10 +297,10 @@ func TestMeterCreatesInstruments(t *testing.T) { } ctr, err := m.Float64ObservableUpDownCounter("afloat", instrument.WithFloat64Callback(cback)) assert.NoError(t, err) - _, err = m.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = m.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 11) return nil - }) + }, ctr) assert.NoError(t, err) // Observed outside of a callback, it should be ignored. @@ -329,10 +327,10 @@ func TestMeterCreatesInstruments(t *testing.T) { } gauge, err := m.Float64ObservableGauge("agauge", instrument.WithFloat64Callback(cback)) assert.NoError(t, err) - _, err = m.RegisterCallback([]instrument.Asynchronous{gauge}, func(ctx context.Context) error { + _, err = m.RegisterCallback(func(ctx context.Context) error { gauge.Observe(ctx, 11) return nil - }) + }, gauge) assert.NoError(t, err) // Observed outside of a callback, it should be ignored. @@ -505,19 +503,19 @@ func TestMetersProvideScope(t *testing.T) { m1 := mp.Meter("scope1") ctr1, err := m1.Float64ObservableCounter("ctr1") assert.NoError(t, err) - _, err = m1.RegisterCallback([]instrument.Asynchronous{ctr1}, func(ctx context.Context) error { + _, err = m1.RegisterCallback(func(ctx context.Context) error { ctr1.Observe(ctx, 5) return nil - }) + }, ctr1) assert.NoError(t, err) m2 := mp.Meter("scope2") ctr2, err := m2.Int64ObservableCounter("ctr2") assert.NoError(t, err) - _, err = m1.RegisterCallback([]instrument.Asynchronous{ctr2}, func(ctx context.Context) error { + _, err = m1.RegisterCallback(func(ctx context.Context) error { ctr2.Observe(ctx, 7) return nil - }) + }, ctr2) assert.NoError(t, err) want := metricdata.ResourceMetrics{ @@ -593,17 +591,18 @@ func TestUnregisterUnregisters(t *testing.T) { require.NoError(t, err) var called bool - reg, err := m.RegisterCallback([]instrument.Asynchronous{ + reg, err := m.RegisterCallback( + func(context.Context) error { + called = true + return nil + }, int64Counter, int64UpDownCounter, int64Gauge, floag64Counter, floag64UpDownCounter, floag64Gauge, - }, func(context.Context) error { - called = true - return nil - }) + ) require.NoError(t, err) ctx := context.Background() @@ -646,17 +645,18 @@ func TestRegisterCallbackDropAggregations(t *testing.T) { require.NoError(t, err) var called bool - _, err = m.RegisterCallback([]instrument.Asynchronous{ + _, err = m.RegisterCallback( + func(context.Context) error { + called = true + return nil + }, int64Counter, int64UpDownCounter, int64Gauge, floag64Counter, floag64UpDownCounter, floag64Gauge, - }, func(context.Context) error { - called = true - return nil - }) + ) require.NoError(t, err) data, err := r.Collect(context.Background()) @@ -681,11 +681,11 @@ func TestAttributeFilter(t *testing.T) { if err != nil { return err } - _, err = mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = mtr.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 1.0, attribute.String("foo", "bar"), attribute.Int("version", 1)) ctr.Observe(ctx, 2.0, attribute.String("foo", "bar"), attribute.Int("version", 2)) return nil - }) + }, ctr) return err }, wantMetric: metricdata.Metrics{ @@ -709,11 +709,11 @@ func TestAttributeFilter(t *testing.T) { if err != nil { return err } - _, err = mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = mtr.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 1.0, attribute.String("foo", "bar"), attribute.Int("version", 1)) ctr.Observe(ctx, 2.0, attribute.String("foo", "bar"), attribute.Int("version", 2)) return nil - }) + }, ctr) return err }, wantMetric: metricdata.Metrics{ @@ -737,11 +737,11 @@ func TestAttributeFilter(t *testing.T) { if err != nil { return err } - _, err = mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = mtr.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 1.0, attribute.String("foo", "bar"), attribute.Int("version", 1)) ctr.Observe(ctx, 2.0, attribute.String("foo", "bar"), attribute.Int("version", 2)) return nil - }) + }, ctr) return err }, wantMetric: metricdata.Metrics{ @@ -763,11 +763,11 @@ func TestAttributeFilter(t *testing.T) { if err != nil { return err } - _, err = mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = mtr.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 10, attribute.String("foo", "bar"), attribute.Int("version", 1)) ctr.Observe(ctx, 20, attribute.String("foo", "bar"), attribute.Int("version", 2)) return nil - }) + }, ctr) return err }, wantMetric: metricdata.Metrics{ @@ -791,11 +791,11 @@ func TestAttributeFilter(t *testing.T) { if err != nil { return err } - _, err = mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = mtr.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 10, attribute.String("foo", "bar"), attribute.Int("version", 1)) ctr.Observe(ctx, 20, attribute.String("foo", "bar"), attribute.Int("version", 2)) return nil - }) + }, ctr) return err }, wantMetric: metricdata.Metrics{ @@ -819,11 +819,11 @@ func TestAttributeFilter(t *testing.T) { if err != nil { return err } - _, err = mtr.RegisterCallback([]instrument.Asynchronous{ctr}, func(ctx context.Context) error { + _, err = mtr.RegisterCallback(func(ctx context.Context) error { ctr.Observe(ctx, 10, attribute.String("foo", "bar"), attribute.Int("version", 1)) ctr.Observe(ctx, 20, attribute.String("foo", "bar"), attribute.Int("version", 2)) return nil - }) + }, ctr) return err }, wantMetric: metricdata.Metrics{