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

added platform to SentryEnvelopeItemHeader #4287

Merged
merged 4 commits into from
Mar 26, 2025

Conversation

stefanosiano
Copy link
Member

📜 Description

Added platform to SentryEnvelopeItemHeader
Added platform "android" in ProfileChunk envelope items

💡 Motivation and Context

Implements client side of https://github.com/getsentry/team-ingest/issues/679
We want to rate limit UI and backend profile chunks differently.
In order to avoid having to maintain a list of sdk names, we are required to send the platform in the item header.

💚 How did you test it?

Unit tests

📝 Checklist

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Sorry, something went wrong.

Added platform "android" in ProfileChunk envelope items
Copy link
Contributor

github-actions bot commented Mar 25, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 94079a4

Copy link
Contributor

github-actions bot commented Mar 25, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 395.32 ms 419.93 ms 24.61 ms
Size 1.58 MiB 2.22 MiB 652.83 KiB

Previous results on branch: feat/continuous-profiling-envelope-item-header

Startup times

Revision Plain With Sentry Diff
176d5ec 399.08 ms 518.53 ms 119.45 ms
61e8426 384.78 ms 412.96 ms 28.18 ms
1421a3b 426.76 ms 462.20 ms 35.44 ms

App size

Revision Plain With Sentry Diff
176d5ec 1.58 MiB 2.22 MiB 652.83 KiB
61e8426 1.58 MiB 2.22 MiB 652.84 KiB
1421a3b 1.58 MiB 2.22 MiB 652.83 KiB

@stefanosiano stefanosiano marked this pull request as ready for review March 25, 2025 16:41
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM, left a minor comment!

traceFile.getName());
traceFile.getName(),
null,
"android");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if/how hybrid SDKs are using this, but should this always be "android"?

Copy link

Choose a reason for hiding this comment

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

The platform in the chunk item-type header should always match the platform in the chunk item-type payload.

So maybe instead of hard coding android, using profileChunk.getPlatform() could be better ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@markushi i did it this way so the other item headers are not changed. Also, the same code creates and sends the envelopes of the jvm world, so it cannot be set always to "android"
Regarding hybrid, we will see, but they shouldn't use our captureProfileChunk method, so it should be fine
@viglia 👍

@stefanosiano stefanosiano enabled auto-merge (squash) March 26, 2025 11:51
@stefanosiano stefanosiano merged commit 801e677 into main Mar 26, 2025
34 checks passed
@stefanosiano stefanosiano deleted the feat/continuous-profiling-envelope-item-header branch March 26, 2025 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants