-
Notifications
You must be signed in to change notification settings - Fork 14
fix(crashtracker): fix payload from v1 into v2 #6275
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
|
|
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14ffb6f6a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
nccatoni
left a comment
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 (for @DataDog/system-tests-core) but you should get a review from someone familiar with the feature
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc01770c5e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
mhlidd
left a comment
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.
QQ, otherwise LGTM
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.
My main question is about keeping only one test, is mandatory ?
Having a test on v1 for a given set of tracers versions and one v2 would be easier to understand write and to understand.
Having two tests would also allow us to know when the switch from v1 → v2 happened in every tracer impacted.
EDIT: discussed live:
- it's fine to keep a single test.
- The
assert_crash_reportshould be split into two func (v1 and v2) and called when needed instead of havingv1andv2strings to improve test readability. - NIT: Another test asserting all tracer are using v2 could be added, tracer not migrated would be marked as
missing_featureuntil they migrate
genesor
left a comment
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
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
0c9148f
into
main
Motivation
Having support for both v1 and v2 payload.
Libdatadog now send v2 payload : DataDog/libdatadog#1498
Changes
Fixed the crashtracking tests to handle both v1 (
payload:[]) and v2 (payload:{logs:[]}) payload format so we can update to v2 without breaking the system tests.Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P team