ci(nodejs): lower default shard duration to 300 seconds - #7690
Conversation
|
|
1f3baaa to
d426203
Compare
cbeauchesne
left a comment
There was a problem hiding this comment.
This script is only used by system-tests' own CI and is not used by other repositories.
System-tests is already a significant consumer of GitHub runners. We have tried increasing the level of parallelism in the past, and observed that it amplified issues with external calls, rate limits, and other infrastructure dependencies.
I understand that this change comes together with a reduction in the overall number of jobs, which definitely changes the trade-off. That said, the current CI runtime is already relatively low, around ~12 minutes, and because there is a fixed cost associated with spawning each job, doubling the parallelism won't necessarily translate into cutting the CI time in half.
Could you measure the actual runtime improvement? That would give us the data needed to properly evaluate whether the gain is worth the trade-off.
|
@cbeauchesne We have less runners to begin with for Node for the same amount of time. Worth noting that we are used to a very fast feedback loop in Node, way faster than other languages. This is why for us 10 minutes just for the scenario is too much, we try to always be under 10 minutes total run time. All of this is why I went for 300s and not lower too, so that in system-tests it's more balanced, and in dd-trace we're going for a lower value because we always only run one language so we can be more aggressive. As for actual numbers, working on it. |
d426203 to
532d8ad
Compare
532d8ad to
9b7c91b
Compare
|
@cbeauchesne Results are in, for prod+dev combined we go from 22 minutes to 12 minutes total run time. Still a bit high, but way more reasonable and close enough to our 10 minutes target without too many jobs. I think this is the right balance for now. |
|
Looking at the numbers for other languages (for production), even with 35 shards, we're still way under the next 2 other best languages (.NET at 51 shards and Go at 142 shards). Given that, I think we could push this even further. For example, at 4 minutes we'd be around 10 minutes total run time with 43 shards, and at 3 minutes we'd be around 9 minutes run time with 53 shards. Of course these end up being diminishing return so I wouldn't push lower than 5 minutes right away and instead would look into what else takes up the additional time (why 12 minutes total if the tests are done in 5 minutes?). |
Motivation
PR #7689 reduces redundant Node.js weblog/scenario combinations. That leaves
enough job capacity to shorten the system-tests internal Node.js shard target
from 600 seconds to 300 seconds and improve feedback time.
This PR is stacked on #7689.
Changes
library.
existing Python and C expectations.
Runtime measurements
The comparison uses two successful Node.js-only runs:
They are not strictly apples-to-apples. The 600-second run selected
DEFAULT,TEST_THE_TEST; the 300-second run additionally selectedPARAMETRICand
TRACE_STATS_COMPUTATION_OBFUSCATION_DISABLED. The end-to-end timingsbelow are still useful, but a same-scenario rerun is needed for a controlled
comparison.
Observed end-to-end critical-path improvements:
The 300-second production system-tests workflow actually completed in 13m 25s
because its unsharded parametric job took 13m 06s and became the critical
path. Parametric did not run in the baseline and is unaffected by this change,
so including it produced the misleading 24.1% figure.
The observed cost is 38 to 70 end-to-end jobs and 6h 15m 57s to 6h 50m 23s
of aggregate end-to-end runner time, an increase of 34m 26s (9.2%). That cost
comparison also includes the extra end-to-end scenario in the 300-second run.
All 70 end-to-end shards passed. These are single-run measurements, so normal
CI variance still applies.
Job-count comparison
For the
end_to_end,open_telemetrygroups, per prod/dev invocation:main: 60 jobs at 600 seconds, or 115 at 300 seconds.Both remain below the 256-job ceiling. Once #7689 is merged, system-tests CI
will run approximately 70 Node.js end-to-end jobs across prod and dev at the
new target.
Validation
ruff checkon both modified Python filesgit diff --checkmainand the post-test(nodejs): reduce redundant weblog scenario coverage #7689Node.js metadata
Full pytest collection requires a running Docker daemon, which was unavailable
locally.
Workflow
R&P team
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified? I have the approvalfrom R&P team
build-XXX-imagelabel is presentR&P team
Generated by Codex.