GH-4208 fix GCP Pub/Sub tracer

[lillo42]

Description

#4208

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

[claude]

Review: GH-4208 fix GCP Pub/Sub tracer

Thanks for the fix! I traced through the code and can confirm this correctly resolves #4208.

✅ Correctness — verified

The reordering is genuinely the right fix, not a cosmetic change:

• BrighterTracer.WriteProducerEvent ends by calling new TextContextPropogator().PropogateContext(span.Context, message) (BrighterTracer.cs:1041), which injects traceparent/tracestate into message.Header.
• Parser.ToPubSubMessage then reads message.Header.TraceParent / message.Header.TraceState and copies them into the Pub/Sub message Attributes (Parser.cs:340-347).

So the ordering matters: the header must be populated before it is serialized into the outgoing Pub/Sub message. With the old order the trace context was written to the header only after the snapshot had already been produced, so it never crossed the boundary. Swapping the two lines fixes propagation.

This also brings GcpMessageProducer in line with the other transports — e.g. SqsMessageProducer (AWSSQS/SqsMessageProducer.cs:162) calls WriteProducerEvent before serializing/sending. 👍

🧪 Test coverage — the main gap

The PR checklist ticks "I have added/updated tests as appropriate", but the diff only touches the production file — no test was added. This is exactly the kind of ordering bug that a regression test guards against and that a reviewer can't easily catch by eye. Per the repo's mandatory TDD workflow, a failing-first test should accompany the fix.

A minimal test would:

1. Give the producer a Span with a known ActivityContext.
2. Send a message (delay null).
3. Assert the published PubsubMessage.Attributes contains a traceparent matching the span (this test would fail on the pre-fix ordering and pass after).

There is currently no GcpMessageProducer unit test under tests/Paramore.Brighter.Gcp.Tests/, so this would be net-new coverage worth having.

📝 Minor notes (non-blocking)

• Trailing whitespace: the new blank line (line 59) has trailing spaces — worth trimming to keep the diff clean.
• Batch path unaffected but inconsistent: SendAsync(IAmAMessageBatch, …) (lines 122-136) never calls WriteProducerEvent, so batch sends won't propagate producer trace context either. The existing comment ("rely on underlying client instrumentation") suggests this is intentional and it's out of scope for this bug — just flagging it as related follow-up.

Summary

The one-line swap is correct, minimal, and well-scoped to the reported bug. The only real ask is a regression test to lock in the ordering.

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

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-4208 fix GCP Pub/Sub tracer

Thanks for the fix! I traced the code path and the change is correct and addresses the real root cause of #4208.

✅ Correctness

BrighterTracer.WriteProducerEvent doesn't just emit a span event — it also propagates trace context onto the message header via new TextContextPropogator().PropogateContext(span.Context, message) (BrighterTracer.cs:1041). Parser.ToPubSubMessage then reads message.Header.TraceParent / TraceState / Baggage and serializes them into the Pub/Sub attributes (Parser.cs:340-347, 313).

So the ordering matters: the header must be populated before it's serialized into the PubsubMessage. Swapping the two lines so WriteProducerEvent runs first is exactly right, and this is the immediate (delay == null || Zero) path that both SendAsync and the sync Send/SendWithDelay wrapper funnel through. 👍

⚠️ Missing regression test

The PR checklist marks "added/updated tests" but the diff only touches the producer. There is an existing generated test — When_sending_a_message_should_propagate_activity_context — but it wouldn't have caught this bug and won't guard against a regression:

// Assert
Assert.NotNull(message.Header.TraceParent);
Assert.Equal("brighter=...", message.Header.TraceState);
Assert.Equal("key=value,key2=value2", message.Header.Baggage.ToString());

It asserts on the source message.Header, which gets populated by WriteProducerEvent regardless of ordering — so it passes with and without the fix. The bug lives in what actually crosses the Pub/Sub boundary. A true regression test should assert on the received/consumed message's trace attributes (or on the serialized PubsubMessage attributes). Given CLAUDE.md calls TDD mandatory, it'd be good to add/regenerate a test that consumes the published message and verifies TraceParent/TraceState/Baggage survived the round-trip.

📝 Minor

• Line 59 (the new blank separator line) has trailing whitespace — worth trimming.

ℹ️ Out of scope, but noting

The bulk SendAsync(IAmAMessageBatch, ...) path (GcpMessageProducer.cs:122-135) never calls WriteProducerEvent, so batch sends won't propagate trace context from Span at all (only whatever the header already carries). That's a pre-existing design choice and separate from this fix, but may be worth a follow-up issue if batch producers are expected to propagate telemetry.

Overall: minimal, targeted, correct fix. My only real ask is a regression test that verifies context on the delivered message.