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 predicate to MetricReader and MetricProducer #3566

Merged
merged 32 commits into from
Dec 12, 2023

Conversation

asafm
Copy link
Contributor

@asafm asafm commented Jun 25, 2023

Fixes #3324

Changes

As described in #3324 , there is a need to have the ability to filter out selected (instrument, attribute) pairs at the MetricReader Collect operation. For efficiency reasons, this needs to be pushed down to the MetricProducer(s) - both the SDK's one and the registered external ones. Implementations are encouraged to use the predicate to avoid allocating memory allocation for the resulting data point if the predicate filters it out.

TODO

  • Fill CHANGELOG
  • Fill spec compliance matrix
  • Update TOC of sdk.md

@asafm asafm requested review from a team June 25, 2023 09:59
@asafm
Copy link
Contributor Author

asafm commented Jun 25, 2023

@reyang @jack-berg Following our Zoom call, here is the PR I've prepared to match the proposal detailed in the issue #3324.

@reyang In our discussion you mentioned a different model where we will have a processor which will have an optional filter associated with it, so we can know whether we should stream metric data point to the processor, from the metric producer. You said you will think about it.
In theory we can in the future just add processor "function" which will be executed only on data points matching the filter, so I think this proposal doesn't block that option from happening in the future.

@arminru arminru added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory labels Jun 27, 2023
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@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 Jul 17, 2023
@asafm
Copy link
Contributor Author

asafm commented Jul 17, 2023

I'm working on a big adjustment to it, please don't close it

@asafm
Copy link
Contributor Author

asafm commented Jul 17, 2023

@reyang @jack-berg I've revamped the PR in the last commit, following the filter rules approach. Your feedback is much appreciated.

@github-actions github-actions bot removed the Stale label Jul 18, 2023
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks for driving this, I support this change.

My only comment here is about naming, to open up the possibility to also have predicates and filters on meters as a whole, not individual metrics.

Please consider renaming like:

  • Filter -> InstrumentFilter or MetricFilter
  • Predicate -> InstrumentPredicate or MetricPredicate

etc.

Of course, Meter predicates will be an entirely separate discussion and PR, not to take place here, but I just would like to prevent future naming collisions.

asafm and others added 2 commits July 25, 2023 12:49
@jack-berg
Copy link
Member

jack-berg commented Jul 25, 2023

Does a prototype exist for this anywhere? The contribution guidelines specify that prototypes are required to help evaluate. We recently re-affirmed this expectation in #3574.

I ask because the motivation for this is primarily performance oriented. As we've discussed, there's presumably some performance overhead for a reader accessing the aggregated state of all instruments, and for invoking all the callbacks. The predicate stands to avoid some of this overhead. The iteration of this PR which includes rules seems adds a lot of surface area relative to the original version which just required implementations to expose some way to specify a predicate to readers. I want to know what kind of performance improvement we stand to gain by introducing this new surface area.

To be frank, its a lot of new surface area / concepts for what I currently view as a niche use case.

@asafm
Copy link
Contributor Author

asafm commented Nov 23, 2023

@dashpole Did I add the changelog and compliance matrix correctly?

Copy link

github-actions bot commented Dec 1, 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 Dec 1, 2023
@asafm
Copy link
Contributor Author

asafm commented Dec 5, 2023

@dashpole Did I add the changelog and compliance matrix correctly?

@dashpole
Copy link
Contributor

dashpole commented Dec 5, 2023

Changelog and matrix look correct.

@github-actions github-actions bot removed the Stale label Dec 6, 2023
@asafm
Copy link
Contributor Author

asafm commented Dec 6, 2023

So, what is the next step before merging @dashpole ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Add filter predicate to MetricReader (push-down predicate)
9 participants