ref(outcomes): Remove outcomes aggregator, implement forwarding of metrics as client reports#6107
Merged
Merged
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Dav1dde
force-pushed
the
dav1d/metrics-to-client-reports
branch
from
885efd1 to
d9e4c25
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Dav1dde
force-pushed
the
dav1d/metrics-to-client-reports
branch
from
d9e4c25 to
af245c2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Dav1dde
force-pushed
the
dav1d/metrics-to-client-reports
branch
3 times, most recently
from
d313740 to
4f60062
Compare
Dav1dde
force-pushed
the
dav1d/metrics-to-client-reports
branch
from
4f60062 to
bd38321
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Dav1dde
force-pushed
the
dav1d/metrics-to-client-reports
branch
from
bd38321 to
275f411
Compare
…trics as client reports
Dav1dde
force-pushed
the
dav1d/metrics-to-client-reports
branch
from
275f411 to
e0c4b48
Compare
loewenheim
approved these changes
Jun 22, 2026
loewenheim
left a comment
loewenheim
left a comment
Contributor
There was a problem hiding this comment.
LGTM, with one nit and a suggestion.
|
|
||
| // Linear search is fine, we only have a limited amount of outcomes and volume as this | ||
| // service is only supposed to be used for proxy mode Relay. | ||
| let discaded_event = discarded_events |
Contributor
There was a problem hiding this comment.
Suggested change
| let discaded_event = discarded_events | |
| let discarded_event = discarded_events |
| Outcome::Filtered(_) => &mut client_report.filtered_events, | ||
| Outcome::FilteredSampling(_) => &mut client_report.filtered_sampling_events, | ||
| Outcome::RateLimited(_) => &mut client_report.rate_limited_events, | ||
| let discarded_events: fn(&mut ClientReport) -> &mut _ = match message.outcome { |
Contributor
There was a problem hiding this comment.
Alternatively you could do this entire match inline.
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes outcomes aggregator and also handles client reports via metrics.
The exception is proxy mode, as proxy mode does not resolve project configs, which means outcomes cannot be routed through the metrics aggregator. To ease this, there is a custom service implementation for proxy mode, which is very simple and not as optimized as the metrics aggregator, which is fine for a proxy Relay.