fix(benchmarks): accept the multi-scenario verify.py invocation its docs advertise - #237
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #235.
verify.py's docstring advertised three scenarios, its parser took one, and because the parser isbuilt with
description=__doc__the--helpoutput printed the invocation the file rejected.Implements option 1 from the issue:
nargs="+", with each scenario re-invokingsys.executable,copying the pattern
run.py:bench()already uses.Why not a loop
verify.pycallssentry_sdk.init()and appliessentry_scenarios.PATCHESin-process, andinprocess.pystates the constraint:A loop in one process would run scenario 2 through scenario 1's monkeypatches and report results
that look fine and are wrong, which is worse than the loud error it replaced. The one surviving
comment in
main()points atinprocess.pyso the next reader does not collapse it back.A single scenario still runs in-process, unchanged: the parent has done nothing at that point.
Validation moved up front
Not asked for by #235, and worth flagging as added scope. Option 1 makes a late typo destructive:
verify.py errors_only nopewould otherwise run the first scenario, then fail with a rawCalledProcessErrortraceback. Every scenario is now resolved before anything spawns.This also upgrades the single-scenario path. On
main,verify.py nopedied with a bareKeyError.verify.py a berror: unrecognized arguments: bverify.py errors_only nopeerror: unknown scenario: nope, exit 2, nothing runverify.py nopeKeyErrortracebackerror: unknown scenario: nope, exit 2verify.py offerror: scenario 'off' captures nothingVerification
No test covers
benchmarks/, so the seam is a runnable acceptance check rather than a pytest test:N scenarios in one invocation must equal N separate invocations.
My first version of that check compared raw output and failed. The difference was
errors_only_no_txn'strace_id, which is freshly random per process precisely because thatscenario does not continue the incoming trace. The check was wrong, not the code. It now compares
semantics and additionally asserts that a continued trace keeps its incoming id while a fresh one
differs across processes, which turns the nondeterminism into evidence that the isolation is real.
All four scenarios pass, including
errors_only_skip_txn, which applies a monkeypatch and wouldleak under a loop. Review confirmed independently that running it first does not leak into a
following scenario either.
benchmarks/README.md§8'sverify.pyline now uses the four-scenario form, runs verbatim, andreproduces §5's trade-off table exactly:
ruff format,ruff check,ty, 307 tests andmkdocs build --strictare clean.Review changes
Both axes flagged that
describe_one'sNoneguard was unreachable oncemain()validated. It isremoved properly rather than deleted: a
resolve()helper returns the narrowed builder, so eacherror message exists once and
tykeeps its narrowing (it treatsparser.errorasNoReturn,confirmed before committing to the shape). Comments went from five lines to one.
HEREdroppedtyping.Finalto matchrun.py:21andrun_http.py:24, the positional becamescenariosto matchrun.py:68, anddescribe_onebecamerun_scenariosincedescribe()already sits beside it.Two suggestions declined
RawDescriptionHelpFormatter. Argparse does collapse the docstring, so the example reads badlyin
--help. But all six scripts inbenchmarks/use plainArgumentParser(description=__doc__),so this would break a consistent convention for a cosmetic gain on a pre-existing wart.
check=Trueon the subprocess call. After pre-flight validation a child crash isgenuinely exceptional, and failing fast beats continuing to the next scenario.
Known asymmetry
run.pyspawns a separate worker module (inprocess.py);verify.pyspawns itself, which is whatforces the
len(scenarios) > 1fork. Extracting a worker module for a 140-line script would costmore than it returns, but it is the reason this file has two execution modes where
run.pyhas one.Benchmarks only. No production code, no runtime behaviour.