Review asynchronous trace and metric instrumentation

Review exercise · TypeScript

Advanced45 min

Review brief

A message consumer adds vendor-neutral telemetry around asynchronous processing and a bounded retry loop. Deliveries are at least once, but this exercise focuses on whether telemetry tells the truth across one logical message, its attempts, and final disposition.

Behavioral contract

A valid incoming traceparent is the parent of the processing span; extraction occurs before the span is created.
The processing span stays active through the asynchronous processor and completion logging, and ends only after that work settles.
A failed attempt records its exception and sets error status before the span ends; a successful attempt sets success status.
messages_attempt_total increments exactly once per processing attempt, while messages_final_total increments exactly once per logical message after success or dead-letter disposition.
Metric labels may contain only message_type, attempt_outcome, retryable, and final_disposition. Message, customer, tenant, payload, and trace identifiers are prohibited labels.
Completion logs run in the processing span's active context so the logger can correlate them without manually supplied identifiers.
The retry policy makes at most three attempts; processor errors become retry results until final disposition. Idempotency of business processing is out of scope.

Review instructions

1Trace one failed-then-successful delivery across all four changed files.
2Separate span parentage, span lifetime, context propagation, and metric meaning into distinct findings where their causes differ.
3Prioritize no more than seven findings and anchor each to the narrowest useful line.

Learning objectives

• Trace telemetry lifetime and context through asynchronous completion
• Distinguish attempt from final-outcome metrics
• Review labels for cardinality and privacy

src/message-telemetry-types.tsTypeScript

@@ -0,0 +1,46 @@

1+export interface Message {

2+ id: string;

3+ type: string;

4+ customerEmail: string;

5+ tenantId: string;

6+ headers: Readonly<Record<string, string>>;

7+ payload: unknown;

8+}

10+export interface TraceContext {

11+ traceId: string;

12+}

13+

14+export interface Span {

15+ recordException(error: unknown): void;

16+ setStatus(status: "ok" | "error"): void;

17+ end(): void;

18+}

19+

20+export interface Tracer {

21+ extract(headers: Readonly<Record<string, string>>): TraceContext | undefined;

22+ startSpan(name: string, parent?: TraceContext): Span;

23+ runWithSpan<T>(span: Span, work: () => T): T;

24+ activeTraceId(): string | undefined;

25+}

26+

27+export interface Logger {

28+ info(message: string, fields?: Readonly<Record<string, unknown>>): void;

29+}

30+

31+export interface Counter {

32+ add(value: number, labels: Readonly<Record<string, string | boolean>>): void;

33+}

34+

35+export interface MessageTelemetry {

36+ tracer: Tracer;

37+ logger: Logger;

38+ messagesAttempt: Counter;

39+ messagesFinal: Counter;

40+}

41+

42+export type MessageProcessor = (message: Message) => Promise<void>;

43+

44+export type AttemptResult =

45+ | { kind: "succeeded" }

46+ | { kind: "retry"; error: unknown };

Reference solution

Findings are ordered by priority.

Findings

1. The processing span is created before its remote parent is extracted

Location: src/instrumented-message-handler.ts:18-23

startSpan runs without a parent and only then extracts the incoming headers. The extracted context is reduced to a log boolean and can no longer establish parentage, so each delivery starts a disconnected trace instead of continuing the producer's trace.

Extract first, then pass the resulting context as the parent when creating the processing span.

2. The span ends as soon as asynchronous work is scheduled

Location: src/instrumented-message-handler.ts:25-55

The promise continuation contains the processor and completion telemetry, but lines 53-54 mark and end the span before that continuation settles. Real processing latency is excluded, and exceptions and logs occur after the span is already closed.

Run the asynchronous attempt inside the span lifecycle and set status and end the span only after awaiting success or failure.

3. Completion work never runs in the span's active context

Location: src/instrumented-message-handler.ts:25-31

The continuation is attached directly with then and never uses tracer.runWithSpan. Consequently the processor and its completion log do not inherit the processing context; activeTraceId can be absent and downstream spans or automatically correlated logs become detached.

Wrap the entire asynchronous callback with the tracer's context API, ensuring that the API preserves context through the returned promise.

4. Metric labels contain prohibited sensitive and unbounded identifiers

Location: src/instrumented-message-handler.ts:32-49 and src/message-delivery.ts:18-33

The counters attach message IDs, customer email addresses, and tenant IDs. These values create a series per message or customer and expose identifiers through the metrics backend, violating both the cardinality and privacy contract.

Use only the permitted bounded labels: message type, attempt outcome, retryability, and final disposition. Keep identifiers in access-controlled logs or traces where policy permits them.

5. The final-outcome counter is incremented for attempts and again for disposition

Location: src/instrumented-message-handler.ts:32-49 and src/message-delivery.ts:14-33

Each attempt writes to messagesFinal, then the delivery loop writes to the same counter once more when it succeeds or dead-letters. A failed-then-successful message produces retry, success, and another success in a counter that should describe one final logical outcome; the dedicated attempt counter is never used.

Increment messagesAttempt exactly once in the attempt handler and messagesFinal exactly once after the retry loop chooses the logical message's disposition.

6. Recorded exceptions leave the span marked successful

Location: src/instrumented-message-handler.ts:41-54

The catch branch records an exception but never sets error status. The unconditional setStatus("ok") already ran before the failure is observed, so even if a tracing implementation accepts the late exception, the span still reports success.

Set error status in the failure path before ending the span and set success only after the processor resolves.

7. The test inspects telemetry before asynchronous completion

Location: src/instrumented-message-handler.test.ts:41-46

The test discards the handler promise, asserts that the span already ended, and only then resolves the processor. It therefore enshrines the premature lifecycle and never observes completion context, exception status, or the attempt and final counter values after work settles.

Await the handler or delivery promise, use deferred processing to assert the span remains open mid-flight, then inspect telemetry after success and failure completion.

Reasonable non-findings

The retry loop makes at most three attempts and stops immediately after success, as required.
Converting processor exceptions into retry results is appropriate for this bounded delivery workflow; the telemetry around that result is defective.
Message identifiers in the structured completion log are not metric labels and are not prohibited by this exercise's logging contract.
Starting the attempt through a promise continuation is acceptable if tracing context and span ownership are carried through that continuation.
Vendor-specific span names, counter names, and trace-header parsing details are intentionally outside the contract.