From 69611bd01a355466f324cca84068f54e6e388654 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 25 Aug 2023 07:39:43 -0700 Subject: [PATCH] Switch `Stream` back to having an `AttributeFilter` field and add `New*Filter` functions (#4444) * Add allow/deny attr filters * Revert "Replace `Stream.AttributeFilter` with `AllowAttributeKeys` (#4288)" This reverts commit 1633c74aea5f5b9f05083d255d3c37e2b1412e79. * Rename new attr filter funcs Do not include the term "Attribute" in a creation function of the "attribute" pkg. * Update the AttributeFilter field documentation * Add tests for filter creation funcs * Add change to changelog * Apply feedback * Use NewDenyKeysFilter for allow-all and deny-list filters * Remove links from field docs These links do not render. --------- Co-authored-by: Chester Cheung --- CHANGELOG.md | 4 +- attribute/filter.go | 60 +++++++++++++++++++++++++ attribute/filter_test.go | 87 ++++++++++++++++++++++++++++++++++++ attribute/set.go | 7 --- sdk/metric/benchmark_test.go | 2 +- sdk/metric/instrument.go | 31 +++---------- sdk/metric/meter_test.go | 9 ++-- sdk/metric/pipeline.go | 4 +- sdk/metric/view.go | 10 ++--- sdk/metric/view_test.go | 29 +++++++----- 10 files changed, 185 insertions(+), 58 deletions(-) create mode 100644 attribute/filter.go create mode 100644 attribute/filter_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 774ec6edb82..12acf1a39c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Expand the set of units supported by the prometheus exporter, and don't add unit suffixes if they are already present in `go.opentelemetry.op/otel/exporters/prometheus` (#4374) - Move the `Aggregation` interface and its implementations from `go.opentelemetry.io/otel/sdk/metric/aggregation` to `go.opentelemetry.io/otel/sdk/metric`. (#4435) - The exporters in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` support the `OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION` environment variable. (#4437) +- Add the `NewAllowKeysFilter` and `NewDenyKeysFilter` functions to `go.opentelemetry.io/otel/attribute` to allow convenient creation of allow-keys and deny-keys filters. (#4444) ### Changed @@ -37,9 +38,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Count the Collect time in the PeriodicReader timeout. (#4221) - `New` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` returns `*Exporter` instead of `"go.opentelemetry.io/otel/sdk/metric".Exporter`. (#4272) - `New` in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp` returns `*Exporter` instead of `"go.opentelemetry.io/otel/sdk/metric".Exporter`. (#4272) -- ⚠️ Metrics SDK Breaking ⚠️ : the `AttributeFilter` fields of the `Stream` from `go.opentelemetry.io/otel/sdk/metric` is replaced by the `AttributeKeys` field. - The `AttributeKeys` fields allows users to specify an allow-list of attributes allowed to be recorded for a view. - This change is made to ensure compatibility with the OpenTelemetry specification. (#4288) - If an attribute set is omitted from an async callback, the previous value will no longer be exported. (#4290) - If an attribute set is Observed multiple times in an async callback, the values will be summed instead of the last observation winning. (#4289) - Allow the explicit bucket histogram aggregation to be used for the up-down counter, observable counter, observable up-down counter, and observable gauge in the `go.opentelemetry.io/otel/sdk/metric` package. (#4332) diff --git a/attribute/filter.go b/attribute/filter.go new file mode 100644 index 00000000000..638c213d59a --- /dev/null +++ b/attribute/filter.go @@ -0,0 +1,60 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package attribute // import "go.opentelemetry.io/otel/attribute" + +// Filter supports removing certain attributes from attribute sets. When +// the filter returns true, the attribute will be kept in the filtered +// attribute set. When the filter returns false, the attribute is excluded +// from the filtered attribute set, and the attribute instead appears in +// the removed list of excluded attributes. +type Filter func(KeyValue) bool + +// NewAllowKeysFilter returns a Filter that only allows attributes with one of +// the provided keys. +// +// If keys is empty a deny-all filter is returned. +func NewAllowKeysFilter(keys ...Key) Filter { + if len(keys) <= 0 { + return func(kv KeyValue) bool { return false } + } + + allowed := make(map[Key]struct{}) + for _, k := range keys { + allowed[k] = struct{}{} + } + return func(kv KeyValue) bool { + _, ok := allowed[kv.Key] + return ok + } +} + +// NewDenyKeysFilter returns a Filter that only allows attributes +// that do not have one of the provided keys. +// +// If keys is empty an allow-all filter is returned. +func NewDenyKeysFilter(keys ...Key) Filter { + if len(keys) <= 0 { + return func(kv KeyValue) bool { return true } + } + + forbid := make(map[Key]struct{}) + for _, k := range keys { + forbid[k] = struct{}{} + } + return func(kv KeyValue) bool { + _, ok := forbid[kv.Key] + return !ok + } +} diff --git a/attribute/filter_test.go b/attribute/filter_test.go new file mode 100644 index 00000000000..c668e260b83 --- /dev/null +++ b/attribute/filter_test.go @@ -0,0 +1,87 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package attribute + +import "testing" + +func TestNewAllowKeysFilter(t *testing.T) { + keys := []string{"zero", "one", "two"} + attrs := []KeyValue{Int(keys[0], 0), Int(keys[1], 1), Int(keys[2], 2)} + + t.Run("Empty", func(t *testing.T) { + empty := NewAllowKeysFilter() + for _, kv := range attrs { + if empty(kv) { + t.Errorf("empty NewAllowKeysFilter filter accepted %v", kv) + } + } + }) + + t.Run("Partial", func(t *testing.T) { + partial := NewAllowKeysFilter(Key(keys[0]), Key(keys[1])) + for _, kv := range attrs[:2] { + if !partial(kv) { + t.Errorf("partial NewAllowKeysFilter filter denied %v", kv) + } + } + if partial(attrs[2]) { + t.Errorf("partial NewAllowKeysFilter filter accepted %v", attrs[2]) + } + }) + + t.Run("Full", func(t *testing.T) { + full := NewAllowKeysFilter(Key(keys[0]), Key(keys[1]), Key(keys[2])) + for _, kv := range attrs { + if !full(kv) { + t.Errorf("full NewAllowKeysFilter filter denied %v", kv) + } + } + }) +} + +func TestNewDenyKeysFilter(t *testing.T) { + keys := []string{"zero", "one", "two"} + attrs := []KeyValue{Int(keys[0], 0), Int(keys[1], 1), Int(keys[2], 2)} + + t.Run("Empty", func(t *testing.T) { + empty := NewDenyKeysFilter() + for _, kv := range attrs { + if !empty(kv) { + t.Errorf("empty NewDenyKeysFilter filter denied %v", kv) + } + } + }) + + t.Run("Partial", func(t *testing.T) { + partial := NewDenyKeysFilter(Key(keys[0]), Key(keys[1])) + for _, kv := range attrs[:2] { + if partial(kv) { + t.Errorf("partial NewDenyKeysFilter filter accepted %v", kv) + } + } + if !partial(attrs[2]) { + t.Errorf("partial NewDenyKeysFilter filter denied %v", attrs[2]) + } + }) + + t.Run("Full", func(t *testing.T) { + full := NewDenyKeysFilter(Key(keys[0]), Key(keys[1]), Key(keys[2])) + for _, kv := range attrs { + if full(kv) { + t.Errorf("full NewDenyKeysFilter filter accepted %v", kv) + } + } + }) +} diff --git a/attribute/set.go b/attribute/set.go index b976367e46d..9f9303d4f15 100644 --- a/attribute/set.go +++ b/attribute/set.go @@ -39,13 +39,6 @@ type ( iface interface{} } - // Filter supports removing certain attributes from attribute sets. When - // the filter returns true, the attribute will be kept in the filtered - // attribute set. When the filter returns false, the attribute is excluded - // from the filtered attribute set, and the attribute instead appears in - // the removed list of excluded attributes. - Filter func(KeyValue) bool - // Sortable implements sort.Interface, used for sorting KeyValue. This is // an exported type to support a memory optimization. A pointer to one of // these is needed for the call to sort.Stable(), which the caller may diff --git a/sdk/metric/benchmark_test.go b/sdk/metric/benchmark_test.go index dd75de3cd63..90f88088630 100644 --- a/sdk/metric/benchmark_test.go +++ b/sdk/metric/benchmark_test.go @@ -42,7 +42,7 @@ var viewBenchmarks = []struct { "AttrFilterView", []View{NewView( Instrument{Name: "*"}, - Stream{AllowAttributeKeys: []attribute.Key{"K"}}, + Stream{AttributeFilter: attribute.NewAllowKeysFilter("K")}, )}, }, } diff --git a/sdk/metric/instrument.go b/sdk/metric/instrument.go index c09c89361c6..f7224d4b581 100644 --- a/sdk/metric/instrument.go +++ b/sdk/metric/instrument.go @@ -146,31 +146,14 @@ type Stream struct { Unit string // Aggregation the stream uses for an instrument. Aggregation Aggregation - // AllowAttributeKeys are an allow-list of attribute keys that will be - // preserved for the stream. Any attribute recorded for the stream with a - // key not in this slice will be dropped. + // AttributeFilter is an attribute Filter applied to the attributes + // recorded for an instrument's measurement. If the filter returns false + // the attribute will not be recorded, otherwise, if it returns true, it + // will record the attribute. // - // If this slice is empty, all attributes will be kept. - AllowAttributeKeys []attribute.Key -} - -// attributeFilter returns an attribute.Filter that only allows attributes -// with keys in s.AttributeKeys. -// -// If s.AttributeKeys is empty an accept-all filter is returned. -func (s Stream) attributeFilter() attribute.Filter { - if len(s.AllowAttributeKeys) <= 0 { - return func(kv attribute.KeyValue) bool { return true } - } - - allowed := make(map[attribute.Key]struct{}) - for _, k := range s.AllowAttributeKeys { - allowed[k] = struct{}{} - } - return func(kv attribute.KeyValue) bool { - _, ok := allowed[kv.Key] - return ok - } + // Use NewAllowKeysFilter from "go.opentelemetry.io/otel/attribute" to + // provide an allow-list of attribute keys here. + AttributeFilter attribute.Filter } // instID are the identifying properties of a instrument. diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index edb1a400b2d..7c1d21f96cd 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -1518,7 +1518,7 @@ func testAttributeFilter(temporality metricdata.Temporality) func(*testing.T) { WithReader(rdr), WithView(NewView( Instrument{Name: "*"}, - Stream{AllowAttributeKeys: []attribute.Key{"foo"}}, + Stream{AttributeFilter: attribute.NewAllowKeysFilter("foo")}, )), ).Meter("TestAttributeFilter") require.NoError(t, tt.register(t, mtr)) @@ -1565,8 +1565,11 @@ func TestObservableExample(t *testing.T) { selector := func(InstrumentKind) metricdata.Temporality { return temp } reader := NewManualReader(WithTemporalitySelector(selector)) - noFiltered := NewView(Instrument{Name: instName}, Stream{Name: instName}) - filtered := NewView(Instrument{Name: instName}, Stream{Name: filteredStream, AllowAttributeKeys: []attribute.Key{"pid"}}) + allowAll := attribute.NewDenyKeysFilter() + noFiltered := NewView(Instrument{Name: instName}, Stream{Name: instName, AttributeFilter: allowAll}) + + filter := attribute.NewDenyKeysFilter("tid") + filtered := NewView(Instrument{Name: instName}, Stream{Name: filteredStream, AttributeFilter: filter}) mp := NewMeterProvider(WithReader(reader), WithView(noFiltered, filtered)) meter := mp.Meter(scopeName) diff --git a/sdk/metric/pipeline.go b/sdk/metric/pipeline.go index d76231cff7f..c1597a75597 100644 --- a/sdk/metric/pipeline.go +++ b/sdk/metric/pipeline.go @@ -351,9 +351,7 @@ func (i *inserter[N]) cachedAggregator(scope instrumentation.Scope, kind Instrum b := aggregate.Builder[N]{ Temporality: i.pipeline.reader.temporality(kind), } - if len(stream.AllowAttributeKeys) > 0 { - b.Filter = stream.attributeFilter() - } + b.Filter = stream.AttributeFilter in, out, err := i.aggregateFunc(b, stream.Aggregation, kind) if err != nil { return aggVal[N]{0, nil, err} diff --git a/sdk/metric/view.go b/sdk/metric/view.go index 2d0fe18d7e9..e4f350e1912 100644 --- a/sdk/metric/view.go +++ b/sdk/metric/view.go @@ -107,11 +107,11 @@ func NewView(criteria Instrument, mask Stream) View { return func(i Instrument) (Stream, bool) { if matchFunc(i) { return Stream{ - Name: nonZero(mask.Name, i.Name), - Description: nonZero(mask.Description, i.Description), - Unit: nonZero(mask.Unit, i.Unit), - Aggregation: agg, - AllowAttributeKeys: mask.AllowAttributeKeys, + Name: nonZero(mask.Name, i.Name), + Description: nonZero(mask.Description, i.Description), + Unit: nonZero(mask.Unit, i.Unit), + Aggregation: agg, + AttributeFilter: mask.AttributeFilter, }, true } return Stream{}, false diff --git a/sdk/metric/view_test.go b/sdk/metric/view_test.go index 07f0c906cb8..d9e3f9b6c81 100644 --- a/sdk/metric/view_test.go +++ b/sdk/metric/view_test.go @@ -404,18 +404,6 @@ func TestNewViewReplace(t *testing.T) { } }, }, - { - name: "AttributeKeys", - mask: Stream{AllowAttributeKeys: []attribute.Key{"test"}}, - want: func(i Instrument) Stream { - return Stream{ - Name: i.Name, - Description: i.Description, - Unit: i.Unit, - AllowAttributeKeys: []attribute.Key{"test"}, - } - }, - }, { name: "Complete", mask: Stream{ @@ -442,6 +430,23 @@ func TestNewViewReplace(t *testing.T) { assert.Equal(t, test.want(completeIP), got) }) } + + // Go does not allow for the comparison of function values, even their + // addresses. Therefore, the AttributeFilter field needs an alternative + // testing strategy. + t.Run("AttributeFilter", func(t *testing.T) { + allowed := attribute.String("key", "val") + filter := func(kv attribute.KeyValue) bool { + return kv == allowed + } + mask := Stream{AttributeFilter: filter} + got, match := NewView(completeIP, mask)(completeIP) + require.True(t, match, "view did not match exact criteria") + require.NotNil(t, got.AttributeFilter, "AttributeFilter not set") + assert.True(t, got.AttributeFilter(allowed), "wrong AttributeFilter") + other := attribute.String("key", "other val") + assert.False(t, got.AttributeFilter(other), "wrong AttributeFilter") + }) } type badAgg struct {