Skip to content

ref(managed): Implement Managed::zip function - #6174

Open
pierceroberts wants to merge 6 commits into
getsentry:masterfrom
pierceroberts:proberts-impl-managed-zip-fn
Open

ref(managed): Implement Managed::zip function#6174
pierceroberts wants to merge 6 commits into
getsentry:masterfrom
pierceroberts:proberts-impl-managed-zip-fn

Conversation

@pierceroberts

Copy link
Copy Markdown
Contributor

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.

@pierceroberts
pierceroberts requested a review from a team as a code owner July 4, 2026 09:20
@pierceroberts pierceroberts changed the title (ref): Implement Managed::zip function in envelope fns (ref): Implement Managed::zip function Jul 6, 2026
Comment thread relay-server/src/endpoints/minidump.rs Outdated
@pierceroberts
pierceroberts force-pushed the proberts-impl-managed-zip-fn branch from cd5bb4a to 10a6fe8 Compare July 6, 2026 05:28
@pierceroberts

Copy link
Copy Markdown
Contributor Author

@Dav1dde Please take a look at this implementation for the zip function. Thanks!
Also, Changelog failed again... This time I am pretty sure it's not the title, maybe it's the description?

@olksdr

olksdr commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

@pierceroberts you will have to fix the title, you can see the error

Error: PR title does not match Sentry conventions.
Please follow the Sentry commit message conventions: https://develop.sentry.dev/engineering-practices/commit-messages/

@pierceroberts

pierceroberts commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@pierceroberts you will have to fix the title, you can see the error

Error: PR title does not match Sentry conventions.
Please follow the Sentry commit message conventions: https://develop.sentry.dev/engineering-practices/commit-messages/

Huh weird, that links to commit-message guidelines and only mentions the pr title once... I'll have to do some more digging to figure out what is wrong with the title.
Alright fixed the title, now looks like it's a changelog issue.

@pierceroberts pierceroberts changed the title (ref): Implement Managed::zip function ref(managed):Implement Managed::zip function Jul 7, 2026
@pierceroberts pierceroberts changed the title ref(managed):Implement Managed::zip function ref(managed): Implement Managed::zip function Jul 7, 2026
@pierceroberts

Copy link
Copy Markdown
Contributor Author

@olksdr I fixed the failing pipeline. I did add the log to the unreleased section. Let me know if I need to move that. Thanks!

@pierceroberts

Copy link
Copy Markdown
Contributor Author

MIght be a flaky test in the integration test suit:

FAILED tests/integration/test_metrics.py::test_transaction_metrics_extraction_external_relays[False] - AssertionError: assert 3 == 4
 +  where 3 = len({'discarded_events': [], 'filtered_sampling_events': [{'category': 'span_indexed', 'quantity': 2, 'reason': 'Sampled:0'}, {'category': 'transaction_indexed', 'quantity': 1, 'reason': 'Sampled:0'}], 'timestamp': 1783399550})
============ 1 failed, 966 passed, 59 skipped in 451.44s (0:07:31) =============
make: *** [Makefile:75: test-integration] Error 1
Error: Process completed with exit code 2.

If it passes, Should I create an issue for it?

Comment thread CHANGELOG.md Outdated
- Use dedicated secret to sign upload URLs. ([#6132](https://github.com/getsentry/relay/pull/6132))
- Inline small attachments instead of uploading to objectstore. ([#6165](https://github.com/getsentry/relay/pull/6165))
- Retry 500 responses from objectstore. ([#6162](https://github.com/getsentry/relay/pull/6162))
- Implement Managed::zip function. ([#6174](https://github.com/getsentry/relay/pull/6174))

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.

Can remove the changelog now, added the skip-changelog label

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.

Okay I did fix the pipeline. What is the standard here? So I can do it right next time.

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.

Rule of thumb, If it has customer observable impact -> Fix/Feature, if it has any visible impact -> Internal. If it is a non trivial refactor which should not have any impact -> Internal, everything else -> No Changelog.

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 went ahead and deleted this change log entry.

Comment thread relay-server/src/endpoints/minidump.rs Outdated
) -> Result<Managed<Box<Envelope>>, BadStoreRequest> {
let event_id = common::event_id_from_items(&items)
.reject2(&items, &managed_err)?
let merged = Managed::zip(managed_err, items);

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.

This can be simplified to:

    Managed::zip(managed_err, items).try_map(|(_, items), _| {
        let event_id = common::event_id_from_items(&items)?.unwrap_or_default();
        let envelope = Envelope::from_request(Some(event_id), meta).with_items(items);
        Ok::<_, BadStoreRequest>(Box::new(envelope))
    })

Which then we might as well inline and we can drop the helper.

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.

By dropping the helper, are you referring to the map function? or zip?

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.

I meant the fn envelope helper function, with zip + try_map I feel like we can inline it without having to go through that function.

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.

Alright I made this change would you like me to create another issue to remove the envelope function? Or would you still like to noodle on it?

@pierceroberts

Copy link
Copy Markdown
Contributor Author

Apologies, I was out on holiday and am getting ready to go back to work. I'll try to get working on this again.

Comment thread relay-server/src/endpoints/playstation.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.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 34504d8. Configure here.

Comment thread relay-server/src/endpoints/playstation.rs Outdated
@pierceroberts
pierceroberts requested a review from Dav1dde August 3, 2026 03:41
@pierceroberts

Copy link
Copy Markdown
Contributor Author

@Dav1dde I am not sure what is up with the warden pipeline. But do these changes look good to you. Thanks!

@Dav1dde

Dav1dde commented Aug 3, 2026

Copy link
Copy Markdown
Member

@pierceroberts ah sorry I missed the update on the PR, mind removing the fn envelope helper and just inlining the code? From what it looks like the envelope function is only called once and now a bit obsolete.

Warden is failing because of missing security tokens for external PRs, I reported that but fine to ignore.

@pierceroberts

Copy link
Copy Markdown
Contributor Author

@Dav1dde No worries! Are you good if we merge this and create a new PR to remove the envelope fn or you'd prefer to use this same PR? I can start that this week.

@Dav1dde
Dav1dde enabled auto-merge August 4, 2026 06:45
@Dav1dde

Dav1dde commented Aug 4, 2026

Copy link
Copy Markdown
Member

Sounds like you'd prefer to do it in a separate PR^^, let's get this merged.

@Dav1dde
Dav1dde added this pull request to the merge queue Aug 4, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 4, 2026
@Dav1dde
Dav1dde added this pull request to the merge queue Aug 4, 2026
@Dav1dde
Dav1dde removed this pull request from the merge queue due to a manual request Aug 4, 2026
@Dav1dde

Dav1dde commented Aug 4, 2026

Copy link
Copy Markdown
Member

Interesting somehow a lint is failing now

error: unused import: `relay_event_schema::protocol::EventId`
  --> relay-server/src/endpoints/playstation.rs:10:5
   |
10 | use relay_event_schema::protocol::EventId;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

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