(ref): Implement Managed::zip function#6174
Conversation
pierceroberts
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| let envelope = merged.map(|(_, items), _| { | ||
| Box::new(Envelope::from_request(Some(event_id), meta).with_items(items)) | ||
| }); | ||
| Ok(envelope) |
There was a problem hiding this comment.
Bug: When zipping managed_err and items and then mapping the result, the quantity for DataCategory::Error is discarded, leading to an assertion failure in debug builds and incorrect outcome tracking in release.
Severity: MEDIUM
Suggested Fix
The map operation needs to preserve the quantities from both parts of the zipped Managed object. The object returned from the map closure must correctly represent the quantities of all its original components. This can be achieved by modifying the closure to not discard the error information or by adjusting how quantities are calculated after the map operation, for instance by manually carrying over the quantities from the discarded part.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/endpoints/minidump.rs#L626-L629
Potential issue: The `Managed::zip` function is used to combine `managed_err` (which
accounts for one `DataCategory::Error`) and `items`. The resulting `merged` object is
then processed using `merged.map`. The closure passed to `map` discards the error
information from the zipped tuple and creates a new `Box<Envelope>` containing only the
`items`. Consequently, the quantity tracking for the `DataCategory::Error` is lost. In
debug builds, this will cause an `assert_quantities` panic due to the mismatch. In
release builds, this results in the failure to record an outcome for the processed
error, leading to incorrect metrics.
Also affects:
relay-server/src/endpoints/playstation.rs:232~235
Did we get this right? 👍 / 👎 to inform future reviews.
pierceroberts
force-pushed
the
proberts-impl-managed-zip-fn
branch
from
cd5bb4a to
10a6fe8
Compare
|
@Dav1dde Please take a look at this implementation for the zip function. Thanks! |
1 participant
Relates to #5985.
And implements: #6088
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.