-
-
Notifications
You must be signed in to change notification settings - Fork 192
feat: read and capture envelopes #1320
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
Conversation
|
D:\a\sentry-native\sentry-native\src\sentry_envelope.c(765,53): error : implicit conversion changes signedness: 'long long' to 'unsigned long long' [-Werror,-Wsign-conversion] [C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\cmake0\sentry.vcxproj] D:\a\sentry-native\sentry-native\src\sentry_envelope.c(786,62): error : implicit conversion changes signedness: 'long long' to 'unsigned long long' [-Werror,-Wsign-conversion] [C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\cmake0\sentry.vcxproj] D:\a\sentry-native\sentry-native\src\sentry_envelope.c(803,61): error : implicit conversion changes signedness: 'long long' to 'unsigned long long' [-Werror,-Wsign-conversion] [C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\cmake0\sentry.vcxproj]
a051d7d
to
a9bf358
Compare
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.
Looks alright to me except for a few potential issues below.
Co-authored-by: Ivan Dlugos <6349682+vaind@users.noreply.github.com>
Thanks, good catches! 🙏 I added a bunch of invalid test cases, and also the examples listed at https://develop.sentry.dev/sdk/data-model/envelopes/#full-examples |
@sentry review |
might make cursor satisfied?
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.
Thanks for the changes, the code is much more defensive now
7f7971b
to
fb54090
Compare
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.
LGTM! do you think it makes sense to merge this before merging in the int64
PR (which is currently tied to Logs, but could be merged separately before fully finishing the Logs PR), or should we already have int64
values in this PR?
I'd vote for merging |
I'll fix the TODOs when |
Expose minimal envelope API for crash reporters / feedback handlers. Extracted from #1303.
Semi-pseudo example:
Minimal but complete feedback handler used for integration tests: feedback.c