-
Notifications
You must be signed in to change notification settings - Fork 532
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
feat(profiling): add platform header to the chunk item-type in the envelope #4178
feat(profiling): add platform header to the chunk item-type in the envelope #4178
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #4178 +/- ##
==========================================
- Coverage 79.56% 79.53% -0.04%
==========================================
Files 141 141
Lines 15736 15736
Branches 2675 2675
==========================================
- Hits 12520 12515 -5
- Misses 2369 2376 +7
+ Partials 847 845 -2
|
sentry_sdk/envelope.py
Outdated
Item( | ||
payload=PayloadRef(json=profile_chunk), | ||
type="profile_chunk", | ||
headers={"platform": profile_chunk.get("platform")}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind to avoid any potential None
value here. Can we hardcode the same value we set in profile_chunk
? Or only add the headers if we have a key platform
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phacops I could throw-in an extra check if you wish but just fyi: if profile_chunk.get("platform")
returns None
it means the platform is not set at the payload either and therefore the whole profile would be rejected as it does not comply to the Relay defined schema (platform is a mandatory field).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I'm just allergic to null
values.
We need to send the platform as part of the headers in the chunk item-type as this is the header that relay is checking to manage rate limiting.