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.

Sign up for GitHub

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

Jump to bottom

Add attribute filtering function to View metric data stream configuration #3550

Closed
wants to merge 10 commits into from
Closed

Add attribute filtering function to View metric data stream configuration #3550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d0bc938
Add attr filter func to view config
MrAlias Jun 8, 2023
b989c6e
Add change to changelog
MrAlias Jun 8, 2023
c114b35
Merge branch 'main' into view-attr-filter-func
MrAlias Jun 8, 2023
21c5aac
Update PR number
MrAlias Jun 8, 2023
4377d1b
Specify behavior for multiple attr conf methods
MrAlias Jun 8, 2023
a06f786
Merge branch 'main' into view-attr-filter-func
MrAlias Jun 8, 2023
728a24a
Merge branch 'main' into view-attr-filter-func
MrAlias Jun 9, 2023
db20704
Merge branch 'main' into view-attr-filter-func
MrAlias Jun 26, 2023
0db7055
Merge branch 'main' into view-attr-filter-func
MrAlias Jun 27, 2023
60ece05
Merge branch 'main' into view-attr-filter-func
MrAlias Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ release.

### Metrics

- Add attribute filter function to view metric stream configuration.
([#3550](https://github.com/open-telemetry/opentelemetry-specification/pull/3550))
- Refine SDK MeterProvider configuration section.
([#3522](https://github.com/open-telemetry/opentelemetry-specification/pull/3522))
- Make SDK Meter Creation more normative.
Expand Down
19 changes: 15 additions & 4 deletions specification/metrics/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,21 @@ are the inputs:
stream](./data-model.md#events--data-stream--timeseries):
* The `description`. If not provided, the Instrument `description` MUST be
used by default.
* A list of `attribute keys` (optional). If provided, the attributes that are
not in the list will be ignored. If not provided, all the attribute keys
will be used by default (TODO: once the Hint API is available, the default
behavior should respect the Hint if it is available).
* The attributes to preserve or drop. This MUST be defined using at least
one, but MAY be defined using multiple, of the following ways. If multiple
of the following ways are used in an implementation, the last one
configured by the use MUST take precedence and all others MUST NOT be
applied. If a user provides no value for the way(s) implemented, all
measurement attributes MUST be used (TODO: once the Hint API is available,
the default behavior should respect the Hint if it is available).
* A list of `attribute keys` (optional). If provided, the attributes that
are not in the list will be ignored.
* An attribute filtering function (optional). The filtering function MUST
accept as an argument an attribute key-value and returns a filtering
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question I posted applies here as well. What do we gain from allowing you to filter on key-value pairs as opposed to just the keys?

We could consider taking this a step further and re-adding the "AttributeProcessor" concept, which is defined as:

interface AttributesProcessor {
  Attributes process(Attributes, Context);
}

But if we did this, I'd say we should take it even further, and add a MeasurementProcessor:

interface Measurement {
  Instrument getInstrument();

  double getValue();

  double getAttributes();
}

interface MeasurementProcessor {
  @Nullable
  Measurement process(Measurement, Context);
}

This would allow users to transform attributes before aggregation, including enriching with attributes from context. It would also allow for filtering measurements whose values are outside some acceptable range. It would also allow for converting the units of values.

So more flexibility. I'm in favor of going all the way in terms of flexibility and adding MeasurementProcessor, rather than adding more constrained tools in a piecemeal fashion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of going all the way in terms of flexibility and adding MeasurementProcessor, rather than adding more constrained tools in a piecemeal fashion.

That does look more flexible and useful to users. I am in favor of going that direction as well. I can close this and open something to shift there if others agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch to the MeasurementProcessor, would it overlap with the exiting View attribute key filter? Could we make that part of the view stream configuration parameters optional to implement (assuming they already aren't 🤷 😉 #3524 (comment))?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO I think the big difference here is where the config is located.

I want a MeasurementProcessor, but I also want more flexible attributes processing on views. View come with an implicit filter for where they apply. I'm not sure I want a measurement processor on views or as a global thing.

@jack-berg what's your thinking here for where MeasurementProcessor lives? can I make a global one, like Log/Span processsor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this topic, I support the idea of a MeasurementProcessor. Lightstep has been pushing on an OTel-Collector pipeline recently and one of the features we wanted to inject via the OTel SDK is a way to extend the attribute set w/ context variables. The same functionality should be able to remove attribute-values, (see it in draft form),

Process(ctx context.Context, inAttrs []attribute.KeyValue) (outAttrs []attribute.KeyValue)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jack-berg what's your thinking here for where MeasurementProcessor lives? can I make a global one, like Log/Span processsor?

I was imagining it was a view level configuration, allowing it to apply broadly to many instruments or narrowly to just one.

If we switch to the MeasurementProcessor, would it overlap with the exiting View attribute key filter?

If we make measurement processor a view level config option, we can think of the existing view attribute key filter as shorthand for a measurement processor which only retains certain attribute keys. Would need to decide what to do when a key filter and a measurement processor are configured, as we could either reject that configuration as invalid, or apply one after the other sequentially based on which was registered first.

decision. The filtering decision can be as simple as a boolean value or a
more appropriate response idiomatic to the implementation language. If
provided, the attributes that are passed to the function for which a
filtering decision to drop them is returned will be ignored.
* The `aggregation` (optional) to be used. If not provided, the SDK MUST
apply a [default aggregation](#default-aggregation) configurable on the
basis of instrument kind according to the [MetricReader](#metricreader)
Expand Down