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

Make it impossible to track outcomes for items that remain in the envelope #1888

Closed
5 tasks done
jjbayer opened this issue Feb 28, 2023 · 1 comment
Closed
5 tasks done
Assignees
Labels
filler Requires little effort to resolve. Ready to be picked up anytime.
Milestone

Comments

@jjbayer
Copy link
Member

jjbayer commented Feb 28, 2023

Before #1877, we reported rate limiting outcomes for replay items that remained in the envelope. This was possible because track_outcome uses the output of EnvelopeLimiter::enforce, but that function did not actually remove replays from the envelope:

let (enforcement, rate_limits) = self.execute(&summary, scoping)?;
envelope.retain_items(|item| self.retain_item(item, &enforcement));
Ok((enforcement, rate_limits))

Not sure what's the exact solution here, but generally speaking, any function that creates an outcome should consume the item that has been dropped.

Tasks

Preview Give feedback
  1. jjbayer
@jjbayer jjbayer added the filler Requires little effort to resolve. Ready to be picked up anytime. label Mar 6, 2023
@jjbayer jjbayer added this to the Maintenance milestone Mar 6, 2023
@jjbayer jjbayer self-assigned this Mar 14, 2023
jjbayer added a commit that referenced this issue Mar 17, 2023
Make `EnvelopeContext` the owner of `Envelope`. and rename it to
`ManagedEnvelope`.

In follow-up PRs, this will allow us to:

- Move outcome tracking into `retain_item`.
- Let `ManagedEnvelope` manage all modifications to `Envelope`, such
that the `EnvelopeSummary` is always up-to-date.

ref: #1888
@jjbayer
Copy link
Member Author

jjbayer commented Mar 17, 2023

Outcomes can now be produced right where the envelope items are dropped, but the initial goal of simplifying outcome reporting in rate limits has not been achieved.

Let's take a more top-level look at how to restructure Relay's data flow and the envelope data type (hierarchical containers instead of flat list of items?) instead.

jjbayer added a commit that referenced this issue Mar 20, 2023
Add a function to `ManagedEnvelope` which tightly couples removal of
items with outcome reporting.

This iteration only covers the easy cases (mostly in processing), where
there is a 1-to-1 relationship between dropped item and produced
outcome. The rate limiting case will have to be handled later.

ref: #1888

Co-authored-by: Iker Barriocanal <[email protected]>
@jjbayer jjbayer closed this as completed Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filler Requires little effort to resolve. Ready to be picked up anytime.
Projects
None yet
Development

No branches or pull requests

1 participant