Improving smoke test capabilities - #12051
Conversation
PerfectSlayer
added
type: feature
29804b8 to
817d668
Compare
|
🎯 Code Coverage (details) 🔗 Commit SHA: 8559be0 | Docs | View more details | Give us feedback! |
817d668 to
c659547
Compare
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Bits found no code fix to apply🟢 Investigated · ⚪ No code fix needed
View in Datadog | Reviewed commit e39c84d · Any feedback? Reach out in #deveng-pr-agent |
c659547 to
089af14
Compare
f45d96b to
89f412f
Compare
e39c84d to
adca11c
Compare
d28d501 to
18ce2e4
Compare
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
The PR description and title should be revised to indicate it's more about span links, than the overhaul of the smoke test capabilities which have been handled via earlier PRs.
Codex found two possible issues one on span id decoding, one on the location of the span links in V1 payloads. I'm reporting these because I believe the findings are correct.
18ce2e4 to
0cf64b4
Compare
|
I'll push the last changes from feedback as soon as the document PR is merged |
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The v1 span-link decoder rejects valid unsigned span IDs with bit 63 set. This error stops decoding of the full trace payload.
🤖 Datadog Autotest · Commit 0cf64b4 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Don't you think the final PR of the stack could sum up the whole work? |
0cf64b4 to
3b675f0
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
devflow unqueued this merge request: It did not become mergeable within the expected time |
Having a summary of earlier work is OK, but the PR itself should precisely describe its change it tries to get in the repo not the previous ones. If the commit landing in git is too broad then it misleads. Gicen the title is used in git it should be tailored to the PR's changes. In this case the commit will be "Improving smoke test capabilities" but it really is about span links harness tooling. So yes I think the description should be focused on the changes. And possibly have a specific section that explain the complete set of work required to get there. Alternatively, I think it might be better to use an issue to track work. |
At first, I thought about having another PR to stack for those changes, and keep this PR as the "preview full stack PR", still in draft. But as PR management / reviews become too painful, I did not want to go the clean way. I already baby sit PRs from 8am to 9pm so until we fix it, I'm not sure I will keep doing the "clean way". PRs are already way worst in the repo 😢
Commit log became significantly worse since merge queues… I don't use it anymore and not sure people still care about what is going in 😞 Anyway, I moved all the text behind HTLM comments so I can still read it but it won't be displayed. |
|
@PerfectSlayer Don't worry I get you 😢 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b675f00e7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
acfa565 to
8559be0
Compare
| .message("Unexpected span link count") | ||
| .expected(this.linkMatchers.length) | ||
| .actual(links.size()) | ||
| .buildAndThrow(); |
There was a problem hiding this comment.
If it's throwing unconditionally anyway, why not just use the "throw" keyword?
There was a problem hiding this comment.
I’m using the dedicated fluent API up to the throw. I added a manual throw / return in the other method to help with IntelliJ static checker but there is no other benefits. buildAndThrow() seems the right way to use it according the doc.
| /** | ||
| * Returns the span 64-bit trace identifier, dropping high-order bits if present. |
There was a problem hiding this comment.
That's confusing, why getTraceId returns the span trace id?
There was a problem hiding this comment.
That’s why I documented it. There is no model for trace, only span. So the trace ID is store within span.
Additionally, this trace ID is not the exact one but truncated… Another reason to document the behavior.
Note that I did not change it, I only documented the behavior.
ygree
left a comment
ygree
left a comment
There was a problem hiding this comment.
IDM mostly approves, except for a couple of places where I left notes
4 participants
What Does This Do
This PR contains the follow up changes from all the other PRs of the stack.
In particular:
Motivation
Follow up changes from PR reviews listed in the additional notes.
Additional Notes
Related pull-requests:
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APMLP-1247