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

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

Closed
wants to merge 10 commits into from

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Jun 8, 2023

Fixes #3441

Currently the metric view allows users to specify a set of attribute keys that should be preserved. This limited functionality for attribute filtering with a view impacts the usefulness of views, as pointed out in #3441. Instead of specifying attribute filtering as just an allow-list of attribute keys, allow implementations to accept generalized attribute filtering functions.

These filtering functions can then be directly constructed by users to accommodate their needs and allows attribute filtering function utilities to be built if a SIG or external community wants to standardize on filtering methods other than an allow-list for attribute keys.

These changes remain backwards compatible in that they continue to allow implementations to just accept a list of attribute keys to act as an allow-list.

Prior art

The (non-stable) Go implementation of a view currently implements this filtering functionality with its stream configuration parameter.

specification/metrics/sdk.md Outdated Show resolved Hide resolved
* 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.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 21, 2023
@MrAlias MrAlias removed the Stale label Jun 26, 2023
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 6, 2023
@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 6, 2023

A review by @dashpole in the Go SIG pointed out that having this added to a view will make it problematic to specify a metric signal configuration via a static yaml file.

Closing.

@MrAlias MrAlias closed this Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory Stale
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Support dropping of specific attributes from instruments
5 participants