Skip to content

Commit

Permalink
Switch Stream back to having an AttributeFilter field and add `Ne…
Browse files Browse the repository at this point in the history
…w*Filter` functions (#4444)

* Add allow/deny attr filters

* Revert "Replace `Stream.AttributeFilter` with `AllowAttributeKeys` (#4288)"

This reverts commit 1633c74.

* 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 <[email protected]>
  • Loading branch information
MrAlias and hanyuancheung authored Aug 25, 2023
1 parent f15ae16 commit 69611bd
Show file tree
Hide file tree
Showing 10 changed files with 185 additions and 58 deletions.
4 changes: 1 addition & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)
Expand Down
60 changes: 60 additions & 0 deletions attribute/filter.go
Original file line number Diff line number Diff line change
@@ -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
}
}
87 changes: 87 additions & 0 deletions attribute/filter_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
})
}
7 changes: 0 additions & 7 deletions attribute/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion sdk/metric/benchmark_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ var viewBenchmarks = []struct {
"AttrFilterView",
[]View{NewView(
Instrument{Name: "*"},
Stream{AllowAttributeKeys: []attribute.Key{"K"}},
Stream{AttributeFilter: attribute.NewAllowKeysFilter("K")},
)},
},
}
Expand Down
31 changes: 7 additions & 24 deletions sdk/metric/instrument.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
9 changes: 6 additions & 3 deletions sdk/metric/meter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 1 addition & 3 deletions sdk/metric/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
10 changes: 5 additions & 5 deletions sdk/metric/view.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 17 additions & 12 deletions sdk/metric/view_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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 {
Expand Down

0 comments on commit 69611bd

Please sign in to comment.