nodejs-pubsub icon indicating copy to clipboard operation
nodejs-pubsub copied to clipboard

feat: add support for OTel context propagation and harmonized spans

Open feywind opened this issue 9 months ago • 14 comments

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.)

feywind avatar Sep 20 '23 21:09 feywind

Here is the summary of changes.

You are about to add 2 region tags.
You are about to delete 1 region tag.

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:

  • [ ] Refresh this comment

snippet-bot[bot] avatar Sep 20 '23 21:09 snippet-bot[bot]

This needs a bit more work due to some design changes in the spans.

feywind avatar Oct 16 '23 21:10 feywind

@omBratteng Glad to see a practical use case, thanks for linking :D

feywind avatar Oct 17 '23 17:10 feywind

@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

omBratteng avatar Oct 18 '23 07:10 omBratteng

FYI here are the changes for the latest spec updates, mostly done: https://github.com/feywind/nodejs-pubsub/pull/4

feywind avatar Nov 30 '23 22:11 feywind

FYI here are the changes for the latest spec updates, mostly done: feywind#4

Awesome! Exciting to start using it :)

weyert avatar Dec 14 '23 01:12 weyert

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.

Bishal18 avatar Jan 04 '24 09:01 Bishal18

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.

feywind avatar Jan 05 '24 16:01 feywind

Looking forward to preview release

omBratteng avatar Jan 11 '24 04:01 omBratteng

@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 avatar Feb 22 '24 00:02 naseemkullah

@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.

feywind avatar Mar 21 '24 19:03 feywind

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. :)

feywind avatar Mar 21 '24 20:03 feywind

@hongalex The sample needs updating, but this should more or less be the finished PR now. Thanks for all the review help.

feywind avatar Mar 22 '24 02:03 feywind

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.

feywind avatar Apr 04 '24 19:04 feywind