Review posted successfully on PR #6576. Here's a summary of what I found:

1. HIGH — newChild() at trace.c++:195-196 doesn't pass traceFlags to the constructor, so child spans silently lose the upstream trace flags.

2. MEDIUM — InvocationSpanContext::fromCapnp()/toCapnp() don't handle traceFlags, and the capnp InvocationSpanContext schema lacks the field. Round-trips through capnp lose the flags.

3. MEDIUM — tryFromTraceparent now accepts unsampled traces (flags=0x00) that were previously rejected. This is a behavioral change that could affect callers outside this repo.

4. LOW — SpanContext::operator== now includes traceFlags but InvocationSpanContext::operator== doesn't — inconsistent identity semantics. Posted an inline suggestion to exclude it.

github run

Codecov Report

❌ Patch coverage is 72.26891% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.59%. Comparing base (ce467b6) to head (9a389f3).
⚠️ Report is 6 commits behind head on main.

@@            Coverage Diff             @@
##             main    #6576      +/-   ##
==========================================
- Coverage   66.60%   66.59%   -0.01%     
==========================================
  Files         402      402              
  Lines      115803   115891      +88     
  Branches    19395    19425      +30     
==========================================
+ Hits        77130    77179      +49     
- Misses      27103    27115      +12     
- Partials    11570    11597      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Merging this PR will not alter performance

✅ 72 untouched benchmarks
⏩ 129 skipped benchmarks¹

_{Comparing mar/worker-sampling-4 (9a389f3) with main (7d0a721)}

Review posted successfully on PR #6576. Here's a summary:

The two higher-severity issues from my earlier review have been fixed:

• newChild() now correctly passes traceFlags to the constructor
• InvocationSpanContext::fromCapnp()/toCapnp() now handle traceFlags, and the capnp schema includes the field

One new low-severity issue found:

• Missing test coverage: InvocationSpanContext capnp round-trip with traceFlags and newChild() propagation aren't tested. SpanContext has both, but InvocationSpanContext does not. Posted inline with suggested test code.

The two items from the prior review (operator== inconsistency and behavioral note about unsampled traces) still have their existing inline comments and remain relevant.

github run

Good, getInvocationSpanContext() returns a reference to a member, so taking a reference with auto& is fine.

Now let me evaluate what's actionable:

1. The previous reviews already caught the important issues and the HIGH and MEDIUM bugs were fixed.
2. The remaining items (operator== inconsistency, missing InvocationSpanContext capnp round-trip test for traceFlags, behavioral change note) all have existing inline comments.
3. Let me check if there's anything new the previous reviews missed.

Looking more carefully at the code:

• The kj::mv() on kj::Maybe<uint8_t> is harmless (trivially copyable type, move = copy), but it's a style nit that doesn't affect correctness.
• The capnp schema changes add fields to existing structs using proper union encoding (traceFlags :union { unset, value }), which is wire-compatible since the union defaults to the first variant (unset).
• The InvocationSpanContext simplified constructor (line 196-201) no longer has the parentSpanContext default parameter but now requires traceFlags. This is fine since all callers were updated.

The previous bonk reviews covered the substantive issues well. The remaining open items are:

• operator== inconsistency (LOW, already has inline comment)
• Missing InvocationSpanContext capnp round-trip test with traceFlags (LOW, already has inline comment)
• Behavioral change note about unsampled traces (informational, already has inline comment)

There are no new actionable issues I can identify beyond what was already flagged. The code is correct, the capnp schemas are wire-compatible, and the propagation logic is sound.

LGTM

github run

lgtm other than my one comment

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉