Skip to content
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

feat: add support for OTel context propagation and harmonized spans (v3.x) #1659

Closed
wants to merge 67 commits into from

Conversation

feywind
Copy link
Collaborator

@feywind feywind commented Nov 28, 2022

Work in progress, converting the existing OTel support to use context propagation and more finely grained spans.

Fixes #1389

@feywind feywind added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 28, 2022
@feywind feywind requested review from a team as code owners November 28, 2022 22:51
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: pubsub Issues related to the googleapis/nodejs-pubsub API. size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Nov 28, 2022
@weyert
Copy link
Contributor

weyert commented Dec 8, 2022

@feywind Any way I can help with progressing this PR?

@feywind
Copy link
Collaborator Author

feywind commented Dec 8, 2022

@weyert Hey, glad to see you're interested :D This is an effort to sync across all the supported languages, so it might be a while before it's fully merged/released. When it's a bit further along though, I'd be interested in testing help, since you're one of the people who's more into OTel usage. This is my main project right now, so I'm hoping I can get a preview branch going soon, at the least.

@weyert
Copy link
Contributor

weyert commented Dec 8, 2022

Awesome happy to try out a preview version when it is available :)

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Dec 12, 2022
@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

  • .kokoro/continuous/node12/samples-test.cfg - .kokoro files are templated and should be updated in synthtool
  • .kokoro/presubmit/node12/samples-test.cfg - .kokoro files are templated and should be updated in synthtool
  • .kokoro/samples-test.sh - .kokoro files are templated and should be updated in synthtool

@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Dec 12, 2022
@feywind feywind changed the base branch from main to legacy-v3 August 15, 2023 18:46
@feywind
Copy link
Collaborator Author

feywind commented Aug 15, 2023

I'm going to go ahead and base this off the 3.x branch to get it merged into that, and then I'll up-port it to 4.x.

@@ -32,103 +32,107 @@

const SUBSCRIBER_TIMEOUT = 10;

// [START opentelemetry_tracing]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't do console output and just focus on Cloud Trace as the default sample (since that's also what our open telemetry team's documentation points to).

samples/openTelemetryTracing.js Show resolved Hide resolved
@@ -51,8 +51,8 @@
"@google-cloud/precise-date": "^3.0.0",
"@google-cloud/projectify": "^3.0.0",
"@google-cloud/promisify": "^2.0.0",
"@opentelemetry/api": "^1.0.0",
"@opentelemetry/semantic-conventions": "~1.3.0",
"@opentelemetry/api": "~1.1.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason these are locked down from ^?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it had to do with Node 12 compatibility. I found that there were some semver issues, in practice, though I'll be revisiting this for the 4.x version of the PR, so I'll probably recheck these for 3.x too.

hongalex
hongalex previously approved these changes Sep 7, 2023
@hongalex hongalex dismissed their stale review September 7, 2023 18:02

approved code changes but still need to review tests

@weyert
Copy link
Contributor

weyert commented Sep 13, 2023

Can't wait for this one to get merged. I have run it locally and works well. I haven't been able to run it in the wild as we had some maddening memory issues but that might have been better

I assume it adheres to the messaging outline of opentelemetry? I have to admit I don't fully understand that one

@feywind
Copy link
Collaborator Author

feywind commented Sep 13, 2023

Can't wait for this one to get merged.

You and I both :)

I have run it locally and works well. I haven't been able to run it in the wild as we had some maddening memory issues but that might have been better

Memory issue caused by the OTel PR or something unrelated?

I assume it adheres to the messaging outline of opentelemetry? I have to admit I don't fully understand that one

Such as it is, yeah. The inner workings of the GCP Pub/Sub libraries are represented in the span layout, but I don't think that's particularly specified by OTel. It's our intent to support the standard span attributes and W3C trace propagation attribute.

TBD if the span structure will be identical across client libraries, like C++ has a different thing going that's based on their method call structure.

Given that the 3.x release machinery is just about going, I'm planning to bring this PR up to the main branch at some point and then back-port the finished one to 3.x.

@weyert
Copy link
Contributor

weyert commented Sep 13, 2023

I have run it locally and works well. I haven't been able to run it in the wild as we had some maddening memory issues but that might have been better

Memory issue caused by the OTel PR or something unrelated?

Oh no, work code related issue. Nothing related to this PR sorry for confusing you.

I assume it adheres to the messaging outline of opentelemetry? I have to admit I don't fully understand that one

Such as it is, yeah. The inner workings of the GCP Pub/Sub libraries are represented in the span layout, but I don't think that's particularly specified by OTel. It's our intent to support the standard span attributes and W3C trace propagation attribute.

TBD if the span structure will be identical across client libraries, like C++ has a different thing going that's based on their method call structure.

Yeah, I understood their is this messaging semantic convention they are working on, like https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-spans.md

Makes sense the span structure is the same structure between libraries :))

Given that the 3.x release machinery is just about going, I'm planning to bring this PR up to the main branch at some point and then back-port the finished one to 3.x.

Makes sense but to be honest I can also fully understand it's a v4 thing

@feywind feywind changed the title feat: add proper OpenTelemetry support to Node Pub/Sub feat: add support for OTel context propagation and harmonized spans Sep 20, 2023
@feywind feywind changed the title feat: add support for OTel context propagation and harmonized spans feat: add support for OTel context propagation and harmonized spans (v3.x) Sep 20, 2023
@feywind
Copy link
Collaborator Author

feywind commented Mar 20, 2024

Will re-open this later when the 4.x one is done.

@feywind feywind closed this Mar 20, 2024
@feywind
Copy link
Collaborator Author

feywind commented Sep 11, 2024

I didn't plan it this way 😹 but it's past the year support window for 3.x, and a lot of changes have happened since OTel started, so I won't be doing this back-porting work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTelemetry integration misses the point: we need to propagate the span/context!
5 participants