refactor(ruby): Remove redundant otel instrumentations option - #28
Conversation
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found no code-level issues in this pass. Removing an accepted Buildkite::TestCollector.configure keyword is a breaking configuration-contract change, so this needs human sign-off beyond the repository's L1 approval ceiling.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 25479, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only when repository policy or a verified grant allows it; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical pipeline paths). Human review always required.
There was a problem hiding this comment.
No code-level issues found. I can’t approve because removing an accepted Buildkite::TestCollector.configure keyword is a breaking configuration-contract change beyond this repository’s L1 approval ceiling.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 25505, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
mttns
changed the title
mttns
changed the title
Mention OTEL_RUBY_INSTRUMENTATION_<NAME>_ENABLED=false as the way to skip an instrumentation the bundle requires anyway, now that otel_instrumentations is gone. Drop a spec assertion on registry.install_all that could not fail: the suite-owned path never reaches it, and SDK.configure is already checked.
…mentation Removing otel_instrumentations took away the only switch that skipped every registered instrumentation. The SDK has no global equivalent, so say so, and show the route that does work: configure the SDK yourself without use_all, with OTEL_TRACES_EXPORTER=none so it does not add its default localhost exporter. Spell out how the per-instrumentation variable names are derived.
35649bb to
cc8d85b
Compare
There was a problem hiding this comment.
One documentation edge case is inline. Removing an accepted Buildkite::TestCollector.configure keyword is also a breaking configuration-contract change beyond this repository's L1 approval ceiling, so I'm leaving this as a comment rather than an approval.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 25928, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Co-authored-by: buildsworth-bk-app[bot] <265670805+buildsworth-bk-app[bot]@users.noreply.github.com>
2 participants
Why
The collector used to bundle OpenTelemetry instrumentation gems, and
otel_instrumentationslet users opt out of them. We no longer bundle any: you install and require the instrumentation gems you want, and that alone decides what gets installed. The option has nothing left to control.What
Remove
otel_instrumentations. The collector installs whatever instrumentation the suite has required. If the suite configures the SDK itself, the collector installs nothing, as before.Passing the option now raises
ArgumentError: unknown keyword.