refactor(ts): assemble spans directly, dropping the OpenTelemetry SDK - #77
Conversation
This was referenced Aug 6, 2026
Collaborator
Author
|
This pull request is part of a Mergify stack:
|
Merge Protections🔴 3 of 7 protections blocking · waiting on 👀 reviews and ⛓️ dependency
🔴 ⛓️ Depends-On RequirementsWaiting for
This rule is failing.Requirement based on the presence of
🔴 👀 Review RequirementsWaiting for
This rule is failing.
🔴 🔎 ReviewsWaiting for
This rule is failing.
Show 4 satisfied protections🟢 🤖 Continuous Integration
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 📕 PR description
🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |
kozlek
marked this pull request as ready for review
August 6, 2026 17:31
kozlek
force-pushed
the
devs/kozlek/thomasberdy/mrgfy-8439-rust-api-client/assemble-spans-directly-dropping-opentelemetry-sdk--f702a313
branch
from
August 10, 2026 12:12
963bc8c to
779f607
Compare
Collaborator
Author
Revision history
|
The trace upload moved to the Rust client in the commit below this one, but
spans were still built on the OpenTelemetry SDK and marshalled from
`ReadableSpan`s by a `SpanExporter` shim. That shim and the SDK behind it are
gone: `@mergifyio/ci-core` now assembles the binding's plain span records
directly, as pytest-mergify already does.
`@mergifyio/ci-core` is left with exactly one runtime dependency — the binding.
`@opentelemetry/{api,core,resources,sdk-trace-base}` are dropped from all three
packages.
Ported from the SDK, mirroring pytest-mergify's `tracing.py` so the semantics
are shared rather than re-derived:
* trace/span id generation,
* W3C `traceparent` parsing for `MERGIFY_TRACEPARENT`, keeping the propagator's
rejection rules — bad widths, uppercase or non-hex, all-zero ids, and the
reserved `ff` version,
* span parenting, now an explicit parent id rather than an active context,
* resource attributes as a plain record.
Timestamps are the one place this is more than a transcription. Epoch
nanoseconds are ~1.7e18, past what a JS number holds exactly, so `msToUnixNano`
converts the whole and fractional parts separately in BigInt. A naive
`ms * 1e6` would silently lose precision.
`tracerName` is dropped rather than ported: it set the OTLP instrumentation
scope, and the Rust uploader sends `scope: None`, so it has been inert on the
wire since trace upload moved to the client. Removing it changes no emitted
bytes.
## The test seam
The behavior suites captured spans through an injected `SpanExporter`. That
option becomes `sink`, a `SpanSink` — renamed rather than retyped in place,
because silently changing what a public option accepts is worse than breaking
it. `InMemorySpanSink` replaces `InMemorySpanExporter` and exposes the same
`getFinishedSpans()`, with attributes keyed rather than listed, which is what
the assertions actually read.
These suites are the parity gate for the whole MRGFY-8439 migration, so they
were ported assertion-for-assertion, not re-derived: same span names,
attributes, statuses, parenting and counts. Playwright stayed at 105 tests and
vitest at 20 — unchanged totals, passing against an implementation that shares
no code with the one they were written for.
Two honest deltas in `spans.test.ts`: the "propagates forceFlush errors while
still attempting shutdown" case becomes "propagates an upload failure", since
there is no provider to shut down; and new cases cover the buffer draining on
export and the resource attributes riding along. `trace-context.test.ts` covers
the newly hand-written traceparent parsing and nanosecond conversion.
Fixes MRGFY-8513
Related to MRGFY-8439
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Change-Id: If702a3137ce305d18f35afff305cad4545eeb4a1
kozlek
force-pushed
the
devs/kozlek/thomasberdy/mrgfy-8439-rust-api-client/assemble-spans-directly-dropping-opentelemetry-sdk--f702a313
branch
from
August 11, 2026 15:50
779f607 to
dbda989
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The trace upload moved to the Rust client in the commit below this one, but
spans were still built on the OpenTelemetry SDK and marshalled from
ReadableSpans by aSpanExportershim. That shim and the SDK behind it aregone:
@mergifyio/ci-corenow assembles the binding's plain span recordsdirectly, as pytest-mergify already does.
@mergifyio/ci-coreis left with exactly one runtime dependency — the binding.@opentelemetry/{api,core,resources,sdk-trace-base}are dropped from all threepackages.
Ported from the SDK, mirroring pytest-mergify's
tracing.pyso the semanticsare shared rather than re-derived:
traceparentparsing forMERGIFY_TRACEPARENT, keeping the propagator'srejection rules — bad widths, uppercase or non-hex, all-zero ids, and the
reserved
ffversion,Timestamps are the one place this is more than a transcription. Epoch
nanoseconds are ~1.7e18, past what a JS number holds exactly, so
msToUnixNanoconverts the whole and fractional parts separately in BigInt. A naive
ms * 1e6would silently lose precision.tracerNameis dropped rather than ported: it set the OTLP instrumentationscope, and the Rust uploader sends
scope: None, so it has been inert on thewire since trace upload moved to the client. Removing it changes no emitted
bytes.
The test seam
The behavior suites captured spans through an injected
SpanExporter. Thatoption becomes
sink, aSpanSink— renamed rather than retyped in place,because silently changing what a public option accepts is worse than breaking
it.
InMemorySpanSinkreplacesInMemorySpanExporterand exposes the samegetFinishedSpans(), with attributes keyed rather than listed, which is whatthe assertions actually read.
These suites are the parity gate for the whole MRGFY-8439 migration, so they
were ported assertion-for-assertion, not re-derived: same span names,
attributes, statuses, parenting and counts. Playwright stayed at 105 tests and
vitest at 20 — unchanged totals, passing against an implementation that shares
no code with the one they were written for.
Two honest deltas in
spans.test.ts: the "propagates forceFlush errors whilestill attempting shutdown" case becomes "propagates an upload failure", since
there is no provider to shut down; and new cases cover the buffer draining on
export and the resource attributes riding along.
trace-context.test.tscoversthe newly hand-written traceparent parsing and nanosecond conversion.
Fixes MRGFY-8513
Related to MRGFY-8439
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Depends-On: #76