Skip to content

ref(managed): Add Managed::zip function#6088

Merged
Dav1dde merged 7 commits into
getsentry:masterfrom
pierceroberts:proberts-manage-merge-api
Jun 24, 2026
Merged

ref(managed): Add Managed::zip function#6088
Dav1dde merged 7 commits into
getsentry:masterfrom
pierceroberts:proberts-manage-merge-api

Conversation

@pierceroberts

@pierceroberts pierceroberts commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

#5985

Added a generic Managed::merge helper that consumes two managed values and returns a single Managed<(T, S)>,
transferring outcome responsibility into the merged wrapper while preserving metadata from the first value.

Also added a unit test covering the new behavior: the merged value retains both inner values, emits outcomes
for both through the first managed handle when dropped, and prevents the second managed value from emitting
duplicate outcomes after the merge.

I also ran:

  • pre-commit --files
  • cargo test -p relay-server:
test result: ok. 383 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.67s
  • also ran the new test individually:
   cargo test --package relay-server --lib --all-features -- managed::managed::tests::test_merge_into_tuple --exact --nocapture 

    Finished `test` profile [unoptimized] target(s) in 0.78s
     Running unittests src/lib.rs (target/debug/deps/relay_server-ca0b589ee2561a09)

running 1 test
test managed::managed::tests::test_merge_into_tuple ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 406 filtered out; finished in 0.00s

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.

@pierceroberts pierceroberts requested a review from a team as a code owner June 13, 2026 05:34
Comment thread relay-server/src/managed/managed.rs Outdated
Comment thread relay-server/src/managed/managed.rs Outdated
Comment thread relay-server/src/managed/managed.rs Outdated
Comment thread relay-server/src/managed/managed.rs Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c1e3f13. Configure here.

Comment thread relay-server/src/managed/managed.rs
@pierceroberts

Copy link
Copy Markdown
Contributor Author

Alright I got the main pipelines to pass @Dav1dde and I made the changes. Let me know what you think! Thanks.
It does look like there are some integration tests for python that are failing silently:

requests.exceptions.ConnectionError: Connection refused by Responses - the call doesn't match any registered mock.

https://github.com/getsentry/relay/actions/runs/27785392311/job/82223873618?pr=6088#step:7:166

@Dav1dde Dav1dde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, looks great!

Documenting the behaviour of what happens to the meta is important to me to make it clear it's not an oversight or "random", the debug_assert is useful because we should never merge two Managed instances from different origins.

Comment thread relay-server/src/managed/managed.rs
Comment thread relay-server/src/managed/managed.rs Outdated
@Dav1dde Dav1dde changed the title Init managed::merged function. ref(managed): Add Managed::zip function. Jun 19, 2026
@Dav1dde

Dav1dde commented Jun 19, 2026

Copy link
Copy Markdown
Member

Changed the title to make the CI check happy

@pierceroberts pierceroberts requested a review from Dav1dde June 23, 2026 09:12
@pierceroberts

Copy link
Copy Markdown
Contributor Author

@Dav1dde , I believe I have added the assert check and a test. Give it another look and let me know! Thanks.

Comment thread relay-server/src/managed/managed.rs Outdated
where
S: Counted,
{
assert!(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We basically only do debug_assert!'s in this module, would stick to the same pattern here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I can change this.

Comment thread relay-server/src/managed/managed.rs Outdated
Comment on lines +287 to +293
first
.meta
.as_ref()
.scoping
.eq(&second.meta.as_ref().scoping),
"cannot zip Managed values with different metadata"
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Untested, shouldn't debug_assert_eq!(first.scoping(), second.scoping()) work here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll try it. But yes, probably. Simpler is usually better 😄

Comment thread relay-server/src/managed/managed.rs
@Dav1dde Dav1dde changed the title ref(managed): Add Managed::zip function. ref(managed): Add Managed::zip function Jun 24, 2026

@Dav1dde Dav1dde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! I think only a make format is missing before CI is happy.

@pierceroberts

Copy link
Copy Markdown
Contributor Author

Yeah super weird, I think the rust-analyzer locally is conflicting with the make style-rust lint-rust version. So I have had to some tweaking. Just re-ran it 🤞

@Dav1dde Dav1dde enabled auto-merge June 24, 2026 09:21
@Dav1dde Dav1dde added this pull request to the merge queue Jun 24, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 24, 2026
@Dav1dde Dav1dde added this pull request to the merge queue Jun 24, 2026
Merged via the queue into getsentry:master with commit c14d611 Jun 24, 2026
48 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants