Stricter agent jar validation upon build#11684
Conversation
* Maintains the size check. * Verify some required entries * Ensure there's a minimum number of classes in the whole jar * Ensure products are correctly included and have at least one class * Light size check on the indexes * Fixed list of packages that should not appear in the jar * Run checks as part of the build job, to catch issues earlier
bric3
requested review from
dougqh,
mcculls and
randomanderson
and removed request for
a team
bric3
added
tag: no release notes
|
![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: 74022b7a6a
ℹ️ 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".
🟢 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. |
bric3
changed the title
|
|
||
| // Sanity check on the minimum number of classes; update as needed. Set to about 98% of that number. | ||
| def classCount = entries.keySet().count { it.endsWith('.class') || it.endsWith('.classdata') } | ||
| def classFloor = 17_000 // a bit moe than 98% of 17,279 at time of writing |
There was a problem hiding this comment.
❔ question: Would there be a way to not hardcode this number but to derive it from the project sources (with minimal cost, of course 😬 )?
There was a problem hiding this comment.
Agreed, I don't like the hard numbers there, and quite frankly that's the part I like the less, but that's the cheapest check I found for what the purpose of this check, which is to avoid regressions in the build.
I believe it's doable, however I believe it's probably too complex to be worth it "at this time".
- How to include modules (and their dependencies) that actually end-up in the final jar, which is not always the case within
:dd-java-agent. Maybe with convention plugin something is feasible, but this initiative is still at the early stages. - Also this number acts as a "minimum" threshold, if we derived it could be skewed (build file changes, etc.) without properly noticing.
| Map<String, Long> entries = [:] | ||
|
|
||
| // Jar size ceiling — raise only when the growth is intentional | ||
| def sizeCeiling = 33L * 1024 * 1024 |
There was a problem hiding this comment.
❔ question: Same here, not sure how we can do better to avoid magic numbers here
There was a problem hiding this comment.
In this case I think it's the right call, this number is a threshold. If it's derived from the actual size, we could miss the size increase.
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
How did you come up from the check cases?
Are they based on past issues or mostly come to check most of the tricky parts of the build?
A bit of both. The oltp issue was one of the main driver. And I expanded from that. I also listed a few other items to prevent inadvertent changes.
Also, I anticipate this build file can and will be reworked, and I'd rather err on the cautious side.
dd-octo-sts
Bot
added
the
tag: ai generated
bric3
force-pushed
the
bdu/agent-jar-checks
branch
from
89c32c4 to
74022b7
Compare
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
I came here to comment about the golden instrumentation list (that could introduce toll for limited benefits) but it seems to be gone already? 👀
Sorry, it was meant to be in a stacked PR. So I removed that commit and opened a new PR #11692 |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-mergequeue-cf854d
Bot
merged commit 11c9d48
into
master
3 participants
What Does This Do
Motivation
Improve the safety of publishing a bad agent jar.
Additional Notes
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]