test(ci): type-check and run the custom-observability probes (#867) - #874
Merged
Conversation
The package's four probes ran only when someone ran them by hand. It has no typecheck script, so `pnpm typecheck:all` skipped it silently, and javascript-ci's example step runs `pnpm -F vitest-examples test`, which does not reach it. It gets a tsconfig matching the openai-realtime-demo example, a typecheck script, and its own CI step. The step carries no fork guard, unlike the example step below it: the probes run in-process against an InMemorySpanExporter with no network and no LLM, so a fork PR that breaks the auto-init contract should go red rather than skip. Turning the type check on surfaced a real drift the ticket predicted but did not name. The example pinned @opentelemetry/sdk-trace-base ^1.30.0 while the library depends on 2.7.1, so it handed a 1.x SimpleSpanProcessor to a 2.x setupScenarioTracing. It worked at runtime and did not typecheck. The example is documentation, so it now pins what the library pins. The bigger find is that test-no-auto-init proved nothing. It compared `trace.getTracerProvider().constructor.name` before and after the import, but that call returns the same ProxyTracerProvider instance whether or not anything is registered, so both reads are the string "ProxyTracerProvider" forever. The probe the issue describes as guarding a regression that has shipped before could not observe that regression: with a provider force-registered on import it still printed PASS and exited 0. Registration swaps the proxy's delegate, so the probe now reads the delegate, and asserts the delegate starts as NoopTracerProvider so it cannot go vacuous again from the other end. Evidence per acceptance criterion: - AC1: annotating a tracer as `number` in test-scenario-only.ts turns typecheck:all red; reverting returns it green. - AC3: registering a NodeTracerProvider on import exits 1 with "Provider changed from NoopTracerProvider to NodeTracerProvider". The same mutation against the old probe exited 0 and printed PASS. - AC4: all four probes pass with OPENAI_API_KEY and LANGWATCH_API_KEY unset. typecheck:all, lint:all and lint:lib clean; 1079 library tests unchanged. Closes #867 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
langwatch-agent
added
the
tech-debt-fixer
4 tasks
WalkthroughThe custom observability example now has TypeScript checking, pinned OpenTelemetry dependencies, stronger probe assertions, exporter filter tests, and JavaScript CI coverage. ChangesCustom observability validation
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
javascript/examples/custom-observability/test-no-auto-init.ts (2)
30-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare delegate identity, not only class names.
delegateName()discards the delegate object. The assertion at Line 54 compares strings. If the import replaces oneNoopTracerProviderwith another provider of the same class, the probe reports success although the delegate changed.Retain the delegate objects for equality checks. Derive
constructor.nameonly for logs and failure messages. Verify that the locked OpenTelemetry implementation returns a stable delegate object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/examples/custom-observability/test-no-auto-init.ts` around lines 30 - 54, Update the provider probe around delegateName so it retains the actual delegate objects before and after importing `@langwatch/scenario`, using constructor.name only for logging and failure messages. Compare delegate identity rather than class-name strings, and verify the locked OpenTelemetry implementation returns the same delegate object when no provider is registered.
24-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard
getDelegate()before calling it.
getDelegate()is not on the publicTracerProvidertype. The runtime truthiness check can also call a non-function value. Add an explicit type guard such astypeof provider.getDelegate === 'function'before calling it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/examples/custom-observability/test-no-auto-init.ts` around lines 24 - 28, Update delegateName so the getDelegate member is invoked only when typeof provider.getDelegate is "function"; otherwise use provider directly. Preserve the existing constructor-name return behavior while preventing calls to non-function values.Source: Coding guidelines
javascript/examples/custom-observability/tsconfig.json (1)
19-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheck
scenario.config.mjstype-checking
tsconfig.jsonincludes**/*.mjs, sojavascript/examples/custom-observability/with-config-file/scenario.config.mjsis part of thetsc --noEmitcontract, but semantic JavaScript checking only runs whencheckJs: trueis set. EnablecheckJs, or remove the.mjsinclude if the config file is runtime-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/examples/custom-observability/tsconfig.json` around lines 19 - 23, Update the custom-observability tsconfig so its treatment of JavaScript matches the intended contract: enable checkJs for semantic checking of the included scenario.config.mjs file, or remove the **/*.mjs include if that file is runtime-only. Keep the existing TypeScript include behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/examples/custom-observability/test-no-auto-init.ts`:
- Line 34: Add a workspace-aware lint script for the
custom-observability-example package, or update the root parallel lint command
to exclude that package and openai-realtime-demo. Ensure pnpm lint:all completes
successfully so the subsequent probe step runs.
---
Nitpick comments:
In `@javascript/examples/custom-observability/test-no-auto-init.ts`:
- Around line 30-54: Update the provider probe around delegateName so it retains
the actual delegate objects before and after importing `@langwatch/scenario`,
using constructor.name only for logging and failure messages. Compare delegate
identity rather than class-name strings, and verify the locked OpenTelemetry
implementation returns the same delegate object when no provider is registered.
- Around line 24-28: Update delegateName so the getDelegate member is invoked
only when typeof provider.getDelegate is "function"; otherwise use provider
directly. Preserve the existing constructor-name return behavior while
preventing calls to non-function values.
In `@javascript/examples/custom-observability/tsconfig.json`:
- Around line 19-23: Update the custom-observability tsconfig so its treatment
of JavaScript matches the intended contract: enable checkJs for semantic
checking of the included scenario.config.mjs file, or remove the **/*.mjs
include if that file is runtime-only. Keep the existing TypeScript include
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e67b8267-0829-46c0-81a5-ac1a420a1d8b
⛔ Files ignored due to path filters (1)
javascript/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
.github/workflows/javascript-ci.ymljavascript/examples/custom-observability/package.jsonjavascript/examples/custom-observability/test-no-auto-init.tsjavascript/examples/custom-observability/tsconfig.json
langwatch-agent
commented
Aug 3, 2026
langwatch-agent
commented
Aug 3, 2026
langwatch-agent
left a comment
langwatch-agent
left a comment
Contributor
Author
There was a problem hiding this comment.
1 P1 — a newly required probe exits successfully without asserting its claimed behavior; see inline comment.
langwatch-agent
added
the
hound-checked
… it probes breaks test-scenario-only.ts printed PASS and exited 0 unconditionally, so wiring test:all into CI made a non-asserting script a required gate: a broken run() or tracing setup would still have gone green. The other three probes already exit non-zero on their own failure conditions; this one did not. Turn its observations into gates: the scenario must succeed, scenario spans must exist, and both deliberately-created noise spans must reach the collector, since otherwise "no noise was exported" would be true for a reason unrelated to filtering. The exporter is not reachable from the probe, so the filtering claim is bound where it can be observed: scenarioOnly's selected scope must be the scope the scenario spans are actually emitted under, and must not also select the noise scope. Renaming the instrumentation scope would otherwise turn the filter into a drop-everything rule with nothing to catch it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/examples/custom-observability/test-scenario-only.ts`:
- Around line 152-182: The assertions in the scenarioOnly validation currently
inspect raw InMemorySpanExporter data rather than post-filtered spans. Update
test-scenario-only.ts to import and construct the exporter used after
scenarioOnly filtering, then assert its output contains the expected scenario
spans and no http-server noise spans. Keep memoryExporter.getFinishedSpans()
only for detecting collector span loss, and update the nearby checks around
scenarioOnlyScopes, unselected, and noiseSpans accordingly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e401d56f-4f31-40ff-9072-0e136878addc
📒 Files selected for processing (1)
javascript/examples/custom-observability/test-scenario-only.ts
…their shape filters.test.ts asserted the rule objects deep-equal a literal. Every one of those assertions still passes if the exporter ignores `filters` entirely, so nothing showed that a noise span is dropped, which is the only thing the filters exist to do. LangWatchTraceExporter applies its filters in export() before delegating to the OTLP exporter it extends, so spying on the parent makes the drop observable with no network call and no live endpoint. Two cases: scenarioOnly forwards the scenario scope and drops http-server and next.js, and withCustomScopes forwards the named scope alongside it. Mutation-checked: inverting scenarioOnly's include to exclude, and widening it to admit the noise scope, each fail two of the new cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
javascript/src/tracing/__tests__/filters.test.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required file-level test documentation.
javascript/src/tracing/__tests__/filters.test.tscurrently documents the helper contracts but does not document the test command, coverage expectation, mocked dependencies, or example filter cases. Add this as the first block, before the imports.Use
javascript/package.json:
pnpm -F@langwatch/scenariotestorpnpm -F@langwatch/scenariotest:cifor coverage.- Document
vitestand the mocked parent exporter dependency.- Document an example filter case, such as including
@langwatch/scenariowithscenarioOnly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/src/tracing/__tests__/filters.test.ts` around lines 1 - 4, Add a file-level documentation block before the imports in filters.test.ts describing the test commands from javascript/package.json, including the coverage command, the Vitest framework, the mocked parent exporter dependency, and an example filter case such as including `@langwatch/scenario` via scenarioOnly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@javascript/src/tracing/__tests__/filters.test.ts`:
- Around line 1-4: Add a file-level documentation block before the imports in
filters.test.ts describing the test commands from javascript/package.json,
including the coverage command, the Vitest framework, the mocked parent exporter
dependency, and an example filter case such as including `@langwatch/scenario` via
scenarioOnly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 13e19bed-887a-46d7-bc6f-848ed37be0fe
📒 Files selected for processing (1)
javascript/src/tracing/__tests__/filters.test.ts
Open
3 tasks
langwatch-agent
added
the
ci-green
Contributor
|
Automated low-risk assessment This PR was evaluated against the repository's Low-Risk Pull Requests procedure and does not qualify as low risk.
This PR requires a manual review before merging. |
langwatch-agent
requested review from
sergioestebance
and removed request for
rogeriochaves
August 11, 2026 18:23
rogeriochaves
requested review from
drewdrewthis
and removed request for
sergioestebance
August 11, 2026 18:25
rogeriochaves
approved these changes
Aug 11, 2026
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
2 participants
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.
Ticket
Closes #867.
javascript/examples/custom-observabilityhad no type check and no CI test invocation, so its four probes ran only by hand.Change
A tsconfig matching the
openai-realtime-demoexample, atypecheckscript, and a CI step runningpnpm -F custom-observability-example test:all.The step carries no fork guard, unlike the
Test (Examples)step it sits above. Those probes run in-process against anInMemorySpanExporterwith no network and no LLM, so a fork PR that breaks the auto-init contract should go red rather than skip. AC4 is satisfied in its stronger form: no key is required, so no guard is needed.Two things the type check found the moment it was switched on
1. The example was a major version behind the library. It pinned
@opentelemetry/sdk-trace-base: ^1.30.0(resolving 1.30.1) while the library depends on2.7.1, so it handed a 1.xSimpleSpanProcessorto a 2.xsetupScenarioTracing. It worked at runtime and did not typecheck:The example is documentation, so it now pins exactly what the library pins rather than being cast past. This is the drift the issue anticipated ("the risk is not that they are broken; it is that nothing would tell us when they break") arriving on the first run.
2.
test-no-auto-initproved nothing at all. This is the probe the issue singles out as guarding "a regression that has shipped before".It compared
trace.getTracerProvider().constructor.namebefore and after the import. That call returns the sameProxyTracerProviderinstance whether or not a provider has been registered, so both reads are the string"ProxyTracerProvider"forever and the comparison can never be unequal. Measured:So with a provider force-registered on import, the old probe exited 0 and printed PASS. Wiring that into CI as-is would have bought false confidence, which is worse than the status quo because it reads as coverage.
Registration swaps the proxy's delegate,
NoopTracerProviderto a real provider, so the probe now reads the delegate. It also asserts the delegate starts asNoopTracerProvider, so it cannot go vacuous from the other end either: if anything registers before the comparison, it fails loudly instead of comparing two constants again.Evidence, per acceptance criterion
AC1 — type-checked in CI.
pnpm typecheck:allnow reaches it (examples/custom-observability typecheck: Done). Annotating a tracer asnumberintest-scenario-only.ts:Reverting returns it green.
AC2 — probes run in CI. New
Test (Custom observability probes)step. All four print PASS locally; the run URL will be on this PR's checks.AC3 — they fail loudly on a regression. Registering a
NodeTracerProviderafter the import:Provider changed from NoopTracerProvider to NodeTracerProviderAC4 — no API key required. All four pass with
OPENAI_API_KEYandLANGWATCH_API_KEYunset, hence no fork guard on the step.typecheck:all,lint:allandlint:libclean. The 1079 library tests are unchanged.Notes
The issue describes the lint half as "Fixed in #865", but #865 is still open, so
mainhas nolintscript for this package either. Nothing here depends on it: this addstypecheckand the CI step only. If #865 lands first the two will conflict on thescriptsblock of one smallpackage.json.Also noticed and left alone:
with-config-file/scenario.config.mjsnames an individual in a comment, which the house style rules out. Flagged on the issue rather than folded into a CI-wiring diff.Advisory note
Advisory PR from the tech-debt-fixer bot, for human review, not to be merged by the bot.