GH-4202 Resolve trace resolver

[lillo42]

Description

Related Issues

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the Contributing Guide
• I have checked the documentation for relevant guidance
• I have added/updated XML documentation for any public API changes
• I have added/updated tests as appropriate
• My changes follow the existing code style and conventions

Additional Notes

[claude]

Code Review — GH-4202 Resolve trace resolver

Summary

This is a correct and well-targeted bug fix. 👍

The Tracer helper previously called serviceProvider.GetService<BrighterTracer>(), resolving the concrete type. However the tracer is only ever registered under the interface in BrighterTracerBuilderExtensions.AddBrighterInstrumentation:

var brighterTracer = new BrighterTracer(TimeProvider.System);
services.TryAddSingleton<IAmABrighterTracer>(brighterTracer);

Because nothing is registered against the concrete BrighterTracer, GetService<BrighterTracer>() always returned null. That null was then fed straight into the OutboxProducerMediator construction (ServiceCollectionExtensions.cs:744), so the outbox producer path silently ran without a tracer — no spans/instrumentation. Switching to GetService<IAmABrighterTracer>() is the right fix and makes this resolution consistent with every other tracer resolution in the file (lines 712, 1002, 1009).

Minor nits (non-blocking)

• Trailing whitespace: the new blank line added after the method (line 905) contains trailing spaces. Worth trimming to keep the diff clean.
• Test coverage: no test accompanies the change, and the PR checklist item for tests is unchecked. Per the repo's TDD guidance, a small regression test would guard against this reappearing — e.g. build the service collection with AddBrighterInstrumentation, then assert the resolved mediator / Tracer helper receives a non-null IAmABrighterTracer. This would have caught the original defect.

Other

• No performance or security concerns — this is a pure DI resolution correction.
• (Pre-existing, not introduced here) there's a stray x character in the XML doc comment at line 911; harmless, but easy to clean up if you're already touching this file.

Overall: correct fix, safe to merge. Adding a regression test would strengthen it.

[claude]

Code Review — GH-4202 Resolve trace resolver

Reviewed the diff against the repo conventions. This is a clean, well-scoped bug fix. 👍

Correctness — the fix is right ✅

The change in ServiceCollectionExtensions.cs:

-    return serviceProvider.GetService<BrighterTracer>();
+    return serviceProvider.GetService<IAmABrighterTracer>();

is correct. The tracer is registered against the interface, not the concrete type — BrighterTracerBuilderExtensions.cs:17:

services.TryAddSingleton<IAmABrighterTracer>(brighterTracer);

So the previous GetService<BrighterTracer>() always returned null, silently disabling outbox telemetry. This Tracer() helper was the lone call site using the concrete type — every other resolver in the codebase (ServiceCollectionExtensions.cs:712/1002/1009, the ServiceActivator extensions, and all the BoxProvisioning extensions) already resolves via IAmABrighterTracer. The fix restores consistency.

Test coverage — solid regression test ✅

AddProducers_ResolvesTracerFromInterfaceRegistration is a genuine regression test:

• It registers the tracer only as IAmABrighterTracer, builds the mediator, and asserts the outbox received it.
• The mediator wires outbox.Tracer = tracer (OutboxProducerMediator.cs:148), so before the fix Tracer() returned null and Assert.Same(tracer, ...) would fail — confirming the test actually exercises the bug.
• The reflection read is necessary here, not a code smell: InMemoryOutbox.Tracer is declared { private get; set; } (InMemoryOutbox.cs:112), so the value cannot be read through the public API.

Minor nits (non-blocking)

• Trailing whitespace: the fix adds a blank line with trailing spaces between Tracer(...) and OutboxCircuitBreaker(...). Worth trimming to keep the diff clean.
• Test brittleness: the reflection uses the string literal "Tracer" for the property name — if the property is ever renamed the test silently loses its guarantee only at runtime. Acceptable given the private getter, but nameof(...) isn't usable here since the getter isn't accessible. No action needed.

Summary

Correct, minimal, consistent with existing conventions, and backed by a meaningful regression test. No functional concerns, no performance or security implications. Recommend merge after (optionally) trimming the trailing whitespace.

[codescene-delta-analysis]

Our agent can fix these. Install it.

Gates Passed
4 Quality Gates Passed

Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.

[claude]

Code Review — GH-4202 Resolve trace resolver

Nice, tightly-scoped fix. I traced the root cause and it holds up.

✅ The fix is correct

ServiceCollectionExtensions.Tracer(...) was calling GetService<BrighterTracer>() (the concrete type), but the tracer is only ever registered against the interface:

• BrighterTracerBuilderExtensions.cs:17 -> services.TryAddSingleton<IAmABrighterTracer>(brighterTracer);
• A grep across the whole tree (src/ + tests/) finds no registration of the concrete BrighterTracer type anywhere.

So GetService<BrighterTracer>() always returned null, meaning OutboxProducerMediator received a null tracer and outbox.Tracer (set at OutboxProducerMediator.cs:148) was never populated — the outbox emitted no telemetry. Switching to GetService<IAmABrighterTracer>() fixes this and makes the resolver consistent with the interface lookup already used at ServiceCollectionExtensions.cs:712. Good.

No breaking-change risk: since the concrete type was never resolvable, no existing configuration could have depended on the old behavior.

✅ Test coverage

The added test genuinely exercises the fix — it fails before the change (null tracer) and passes after. The reflection is warranted because InMemoryOutbox.Tracer is declared { private get; set; } (InMemoryOutbox.cs:112), so there is no public getter to assert against.

Minor nits (non-blocking)

• Trailing whitespace: the newly added blank line after the Tracer(...) method contains trailing spaces. Worth trimming to a clean empty line.
• Reflection brittleness: GetProperty("Tracer", ...) couples the test to the property name via a magic string; a rename would silently null out tracerProperty — though the Assert.NotNull(tracerProperty) guards against that, so it is fine as-is.
• PR metadata: the description, Type of Change, and the related-issue link are unfilled. Linking the issue would help traceability.

Overall: correct root-cause fix, minimal blast radius, and a test that actually protects the behavior. 👍