Skip to content

Special path user generated spans#6667

Merged
danlapid merged 1 commit intomainfrom
dlapid/anyUserSpanName
Apr 25, 2026
Merged

Special path user generated spans#6667
danlapid merged 1 commit intomainfrom
dlapid/anyUserSpanName

Conversation

@danlapid
Copy link
Copy Markdown
Collaborator

This will be used internally to ungate some protections.

@danlapid danlapid requested review from a team as code owners April 25, 2026 00:13
@danlapid danlapid requested a review from fhanau April 25, 2026 00:16
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR adds a fromUserCode signal path so that spans created via ctx.tracing.enterSpan can bypass operation-name allowlists in downstream submitters, and caps user-supplied operation names at 64 bytes.

Issues (highest severity first):

  1. [medium] bool fromUserCode constructor parameter violates WD_STRONG_BOOL convention. The codebase convention (documented in AGENTS.md and src/workerd/util/AGENTS.md) is "NEVER use boolean arguments; prefer WD_STRONG_BOOL." The bool fromUserCode = false constructor parameter and member could silently accept wrong-type booleans (e.g. accidentally passing wasAccepted). Suggestion below.

  2. [low] No test coverage for newChildFromUserCode / submitUserSpanOpen distinction or the 64-byte truncation. The existing tracing-helpers-test and tracing-hierarchy-test suites exercise enterSpan end-to-end but don't verify that the submitter sees submitUserSpanOpen (vs submitSpanOpen) or that names > 64 bytes are truncated. This is fine to defer since testing the submitter dispatch likely requires internal infrastructure, but worth noting.

Comment thread src/workerd/io/tracer.h
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 25, 2026

Review posted on PR #6667. Two findings:

  1. [medium] The bool fromUserCode parameter should use WD_STRONG_BOOL(FromUserCode) per codebase convention -- bare booleans are an anti-pattern in workerd. Posted an inline suggestion on tracer.h:252.

  2. [low] No test coverage for the new submitUserSpanOpen dispatch or the 64-byte name truncation. Noted as deferrable since the submitter distinction is primarily consumed by internal infrastructure.

The overall design is sound -- the fromUserCode flag intentionally does not propagate to newChild() children, so runtime sub-operations (e.g. kv_get) nested inside a user span remain subject to normal policies. The SpanObserver::newChildFromUserCode() virtual with a default fallback to newChild() is a clean extension point.

github run

This will be used internally to ungate some protections.
@danlapid danlapid force-pushed the dlapid/anyUserSpanName branch from ac85538 to 2c770fd Compare April 25, 2026 12:30
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 25, 2026

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

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 25, 2026

Merging this PR will degrade performance by 12.72%

❌ 1 regressed benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
Encode_ASCII_32[TextEncoder][0/0/32] 2.7 ms 3.1 ms -12.72%

Comparing dlapid/anyUserSpanName (2c770fd) with main (cadd1aa)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 21.42857% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.52%. Comparing base (cadd1aa) to head (2c770fd).

Files with missing lines Patch % Lines
src/workerd/io/tracer.c++ 30.00% 6 Missing and 1 partial ⚠️
src/workerd/api/tracing.c++ 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6667      +/-   ##
==========================================
- Coverage   66.53%   66.52%   -0.02%     
==========================================
  Files         405      405              
  Lines      115972   115984      +12     
  Branches    19408    19410       +2     
==========================================
- Hits        77160    77155       -5     
- Misses      27230    27244      +14     
- Partials    11582    11585       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@danlapid danlapid merged commit b6ab905 into main Apr 25, 2026
30 of 36 checks passed
@danlapid danlapid deleted the dlapid/anyUserSpanName branch April 25, 2026 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants