Closed feywind closed 1 day ago
Here is the summary of changes.
This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run
label or use the checkbox below:
This needs a bit more work due to some design changes in the spans.
@omBratteng Glad to see a practical use case, thanks for linking :D
@feywind looking forward to this getting merged 😎
If I understand the new changes, there's no attributes added to the receiver span, as it is expected that they are set on the sender side? https://github.com/feywind/nodejs-pubsub/blob/86491230c918dac24eb3f4bf5be8b5005267f8aa/src/telemetry-tracing.ts#L254-L277
FYI here are the changes for the latest spec updates, mostly done: https://github.com/feywind/nodejs-pubsub/pull/4
FYI here are the changes for the latest spec updates, mostly done: feywind#4
Awesome! Exciting to start using it :)
Hi @feywind, by when can i expect a stable release for this? It would be an extremely important piece which would help to avoid unnecessary patching from our end to achieve the context propagation.
Hi @feywind, by when can i expect a stable release for this? It would be an extremely important piece which would help to avoid unnecessary patching from our end to achieve the context propagation.
I completely agree with you. The best I can say for full stable release (which has to include doc updates) is "aiming for 2024Q1". I'm hoping to get a preview release much sooner.
Looking forward to preview release
@feywind @kamalaboulhosn is there anything we can do to help this get merged?
If not, can a smaller incremental change where the w3c context is propagated be merged sooner?
The googclient_OpenTelemetrySpanContext
is not very useful when working with other services that use w3c context.
Thanks!
@naseemkullah The delay has been annoying for sure. The good news is, this is being reviewed in pieces and there will hopefully be a beta release soon.
There's one more of these in the review queue right now: https://github.com/feywind/nodejs-pubsub/pull/5
Some fabulous people on our side have been working to harmonize our span design a bit more with the OTel working group's design in progress for message tracing. :)
@hongalex The sample needs updating, but this should more or less be the finished PR now. Thanks for all the review help.
This is now testable in beta form. I installed in a test project like so:
npm i --save @google-cloud/pubsub@otel-beta
The resulting package.json looks like this:
{
"dependencies": {
"@google-cloud/opentelemetry-cloud-trace-exporter": "^2.1.0",
"@google-cloud/pubsub": "^4.3.3-otel-beta.1",
"@opentelemetry/sdk-trace-node": "^1.23.0",
"@opentelemetry/semantic-conventions": "^1.23.0"
}
}
And then you can take a look at this sample for how to use it from JS, or this sample for how to use it from TS.
We've got more reviewing to do, and merges aren't going to happen for at least a week, but this may at least make it easier to do some testing if you like.
Further updates (for now) will come in as 4.3.3-otel-beta.2
, etc, and will update the tag otel-beta
on npm.
Warning: This pull request is touching the following templated files:
README.md
is managed by synthtool
. However, a partials file can be used to update the README, e.g.: https://github.com/googleapis/nodejs-storage/blob/main/.readme-partials.yamlsamples/README.md
is managed by synthtool
. However, a partials file can be used to update the README, e.g.: https://github.com/googleapis/nodejs-storage/blob/main/.readme-partials.yamlThe otel-beta
tag has been updated to 4.5.0-otel-beta.2
, which is current as of today. This is pretty close now!
This PR adds support for OTel context propagation using the standard W3C headers as Pub/Sub attributes. It also harmonizes the spans we're using with the planned spans in other language libraries.
Fixes https://github.com/googleapis/nodejs-pubsub/issues/1389
(This is the 4.x version of https://github.com/googleapis/nodejs-pubsub/pull/1659 - I'll still be backporting to there, but the changes will go here first.)