Repository navigation
Span-link native export: review fixes for #6129 - #6270
Merged
Merged
Conversation
Add a comment at snapshot_link_string explaining it enforces T_STRING strictly because SpanLink#to_hash pre-stringifies link keys, values, and tracestate, so a non-String is a programming error (unlike the meta setter, which skips non-strings and warns). Remove the stale "5. Populate meta, metrics, and meta_struct" step comment: set_prepared_metastruct runs above it, so the block that follows only populates meta and metrics, and the new set_links block left the sequence reading 1, 2, 3, 5. Align the = column in the link_*_id and method intern blocks to match the ivar-block convention used elsewhere in trace_exporter_init.
The call-site comment restated the rationale already carried by warn_unsupported_fields! and its docstring (span events are not converted, are dropped, and trigger a one-time warning). The call is self-documenting; the rationale belongs in the docstring alone.
Use instance_double(Datadog::Tracing::SpanLink) instead of plain doubles so the to_hash interface is verified and drift is caught. Replace span.instance_variable_set(:@links, ...) with the public Span#links setter, which has no type enforcement, so the test no longer couples to the private ivar name. After the repeated-failure loops in the cleanup specs, perform one valid conversion and assert it succeeds, so a corrupted or half-freed snapshot surfaces as a failure rather than passing silently. Rename "rejects invalid UTF-8 atomically" to "rejects invalid UTF-8": atomicity is guaranteed by libdatadog and covered at batch level in transport_spec, and is not asserted at this level.
…specs Add a conformance link with an empty attribute value to exercise the zero-length branch of snapshot_link_string (ruby_xmalloc(1) with the memcpy skipped), asserting the empty value survives to the wire. This was the one reachable but unexercised branch in the new span-link C path. Use instance_double(Datadog::Tracing::SpanLink) instead of plain doubles so the to_hash interface is verified.
p-datadog
added
the
AI Generated
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: d78f976 | Docs | View more details | Give us feedback! |
…xporter Rewrite the snapshot_link_string comment to state what the T_STRING check enforces. SpanLink#to_hash stringifies attribute values and carries tracestate as a String; attribute keys retain their input type (serialize_attribute returns Symbol keys unchanged), so the prior claim that keys are pre-stringified was inaccurate. Drop the 1./2./3. step-number prefixes from the phase comments in convert_ruby_span_to_rust so all phases read uniformly after the step-5 label was removed. Verified: gcc -fsyntax-only on the isolated comment blocks; C comment delimiters balanced.
Rename the empty-attribute conformance test to describe the observable round-trip rather than the internal byte-copy mechanism. Rename the two tracer_span cleanup tests to state the follow-up valid conversion, so the trailing recovery assertion is self-evident from the description, and remove the comments that narrated it. Verified: bundle exec rake test:native_transport - 146 examples, 0 failures.
p-datadog
marked this pull request as ready for review
September 7, 2026 15:29
p-datadog
requested review from
mabdinur and
marcotc
and removed request for
a team
September 7, 2026 15:29
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaaac10af4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
marcotc
approved these changes
Sep 8, 2026
Base automatically changed from
lloeki/native-span-links-consumer
to
master
September 10, 2026 09:59
gh-worker-ownership-write-b05516
Bot
removed the request for review
from a team
September 10, 2026 09:59
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
4 participants
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.
What does this PR do?
Applies review fixes to the span-link native-exporter work in #6129: adds a conformance test covering empty attribute values, switches span-link test doubles to instance doubles, strengthens the snapshot-cleanup assertions, uses the public
Span#linkssetter in one test, and cleans up comments in the C exporter and Ruby transport.Motivation:
Review of #6129 found test-quality gaps (plain doubles, private-ivar coupling, cleanup assertions that pass even if the C free were a no-op), one reachable but unexercised C branch (
snapshot_link_string's zero-length path), and stale/duplicated comments.Change log entry
None.
Additional Notes:
Stacked on #6129 (base is
lloeki/native-span-links-consumer). Contains the review fixes only.How to test the change?
Unit tests added