-
Notifications
You must be signed in to change notification settings - Fork 362
Add pubsub.delivery_duration_ms to push subscriptions #7205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7205 +/- ##
==========================================
- Coverage 84.56% 77.90% -6.66%
==========================================
Files 532 384 -148
Lines 22656 17739 -4917
==========================================
- Hits 19159 13820 -5339
- Misses 3497 3919 +422 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f36e20970
ℹ️ 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".
| const requestSpanId = attrs['_dd.pubsub_request.span_id'] | ||
| if (requestTraceId && requestSpanId) { | ||
| span.setTag('pubsub.batch.request_trace_id', requestTraceId) | ||
| if (requestSpanId) { | ||
| span.setTag('pubsub.batch.request_span_id', requestSpanId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore batch request trace ID tag
By only emitting pubsub.batch.request_span_id here, the delivery span no longer carries the batch request’s trace ID. That’s not redundant: in producer.js the batch trace ID is derived from spanCtx.toTraceId(true) while the span ID comes from spanCtx.toSpanId(true), so they differ whenever the publish happens under an existing trace (non‑root batch span). Without the trace ID, pubsub.batch.request_span_id becomes ambiguous (span IDs are only unique within a trace), so downstream correlation that relies on the public pubsub.batch.* tags can’t reliably link back to the batch request. Consider restoring pubsub.batch.request_trace_id (or emitting an equivalent public trace ID tag) alongside the span ID.
Useful? React with 👍 / 👎.
Overall package sizeSelf size: 4.39 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.0 | 68.46 kB | 797.03 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BridgeAR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our tests seem to not reflect this change properly.
When I ran the tests locally, the span assertion always returned early and never found a match. It should actually not contain the return. Our test helper would pass, if any trace contains the correct span, which does not seem to be the case.
Please have a look at that and update the test so that it would now need an update due to the changed code.
I just removed the early returns in the tests and now the following all fail:
1) should create BOTH pubsub.delivery span AND HTTP span in same trace
2) should propagate distributed trace context from producer to push delivery
3) should add batch metadata to delivery span
4) should set service name with -pubsub suffix
packages/datadog-plugin-google-cloud-pubsub/src/pubsub-push-subscription.js
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
BenchmarksBenchmark execution time: 2026-01-14 15:56:07 Comparing candidate commit 44d7e36 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 290 metrics, 30 unstable metrics. |
BridgeAR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! The tests are now seemingly running correct, thank you for fixing them! I believe this should be good to land as soon as the type changes are undone
packages/datadog-plugin-google-cloud-pubsub/test/pubsub-push-subscription.spec.js
Show resolved
Hide resolved
* add pubsub.delivery_duration_ms to push subscriptions * update push tests and names of span tags * update tests for push subscriptions * update tests: remove K_SERVICE delete * Update all tests and naming convention of inferred span * create global const for gc * remove ts error check for gc
* add pubsub.delivery_duration_ms to push subscriptions * update push tests and names of span tags * update tests for push subscriptions * update tests: remove K_SERVICE delete * Update all tests and naming convention of inferred span * create global const for gc * remove ts error check for gc
What does this PR do?
Small clean up PR to :
pubsub.batch.request_span_idspan tag which is the same as thepubsub.batch.request_trace_idtag.pubsub.delivery_duration_msto push subscriptions for tag parity with pull subscriptionspubsub.deliverytopubsub.push.receiveMotivation
Plugin Checklist
Additional Notes