fix(profiling): Add platform to envelope item header#18954
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| isChunkFormat?: boolean; | ||
| } | ||
|
|
||
| export function validateProfileItemHeader(header: Record<string, unknown>): void { |
There was a problem hiding this comment.
really sorry for the nitpicking here but I'm curious on your thoughts: I don't particularly like the pattern of these "validator" functions. Here's why:
- I have to jump when debugging tests to get to the actual assertions
- the stack traces when the test fails point to this validator instead of the failed test. Not ideal when the validator is used in a lot of places
Also specifically for this case: We only check that the header includes some properties but we don't assert on its entire content.
I get that the larger validateProfile helper, encapsulates a lot more assertions. Making it more justified. But for this case, with only two assertions, I'd personally avoid DRY and directly add the assertions; or even change it to just one expect(header).toEqual(...) for extra robustness.
Feel free to ignore/overrule me here :D I might be missing a reason for this.
There was a problem hiding this comment.
I used this validator function here to make sure that when testing the headers, this set of assertions is used - so not a crazy reason that would overrule your concerns :D
Yes, it would make sense to just change it to this one-line assertion. The function only makes sense for the very large chunk-testing part.
Changed it to: expect(itemHeader).toEqual({ type: 'profile_chunk', platform: 'javascript' });
Lms24
left a comment
There was a problem hiding this comment.
Thanks for addressing my review!
Adds the missing
platformin the envelope item header.Related: getsentry/sentry-docs#16135
Closes https://linear.app/getsentry/issue/JS-1540/ui-profiling-add-platform-to-profile-chunk-item-header (internal Linear)