feat: Open telemetry integration and span Id fix for nodejs logging library #1497

cindy-peng · 2024-04-13T01:23:09Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #1490 🦕

This change is to add OpenTelemetry support for nodejs-logging library and fix trace-span correlation issue.
More context about this change: https://b.corp.google.com/issues/270985947

Here are the overall and nodejs-logging design doc for this change:
go/logging-otel-span-support
go/logging-otel-span-support-nodejs

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…s-logging into cindy/spanidfix

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…s-logging into cindy/spanidfix

gkevinzheng

Could you put the changes in protos/ in a separate PR? Not sure how related they are to the current changes.

gkevinzheng · 2024-04-24T15:10:02Z

package.json

@@ -45,6 +45,8 @@
    "postpublish": "./.kokoro/publish-min.sh"
  },
  "dependencies": {
+    "@opentelemetry/api": "^1.7.0",


Could you change this to be a peerDependency?

Good catch! Done.

Hi @gkevinzheng,
It looks like our kokoro tests ran on NPM 6.14.18 which doesn't support auto-installment for peer-dependencies: https://github.blog/2020-10-13-presenting-v7-0-0-of-the-npm-cli/#

This is causing system test module not found test failure.

I have posted in JS-Team chat but it doesn't look like there is any solution so far. There is a plan to upgrade kokoro infra to Node18 but not sure when it will be done.

We will probably need to move this back to Dependencies instead of peerDependencies due to this limitation.

Due to the limitation of low NPM version in kokoro system test infra, JS language team suggested us to import OpenTelemetry/api and re-export it for now to avoid test failure and guarantee compatibility : https://chat.google.com/room/AAAAW_Lho4o/ljx6XogZJMI
Once NPM version in Kokoro infra has upgraded, I will change this to peer dependency again.

package.json

src/utils/context.ts

system-test/logging.ts

test/entry.ts

test/utils/context.ts

package.json

daniel-sanche

Mostly LGTM, but see if you can reduce those dependencies

cindy-peng · 2024-04-25T03:20:53Z

Could you put the changes in protos/ in a separate PR? Not sure how related they are to the current changes.

Yea I was trying to remove these as well, but it looks like owlbot committed this automatically to this PR after I removed them: c254669

gkevinzheng

LGTM

cindy-peng · 2024-04-29T06:46:30Z

Could you put the changes in protos/ in a separate PR? Not sure how related they are to the current changes.

Yea I was trying to remove these as well, but it looks like owlbot committed this automatically to this PR after I removed them: c254669

Removed proto changes.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

aabmass

Didn't look at the tests too closely but LGTM from an OTel perspective

aabmass · 2024-05-07T19:05:32Z

package.json

@@ -59,7 +59,8 @@
    "on-finished": "^2.3.0",
    "pumpify": "^2.0.1",
    "stream-events": "^1.0.5",
-    "uuid": "^9.0.0"
+    "uuid": "^9.0.0",
+    "@opentelemetry/api": "^1.7.0"


Would peerDependency work better here?

Yea there was some back and forth discussions here: #1497 (comment)

Basically system test would fail for now if it is imported as peer dependency since our kokoro runs on NPM 6, which doesn't install peer dependency automatically. JS language team suggested us to import OpenTelemetry/api and re-export it for now to avoid test failure and guarantee compatibility : https://chat.google.com/room/AAAAW_Lho4o/ljx6XogZJMI
Once NPM version in Kokoro infra has upgraded, I can change this to peer dependency again.

src/utils/context.ts

cindy-peng added 6 commits February 21, 2024 13:12

feat: OpenTelemetry integration

265a909

feat: add otel system testing with cloud tracing

608c053

Add unit testing for otel context

54d4991

Add parseOtelContext test

d0be991

testing entry metadata population

9b1a5d3

Add system test suites

fd8eb30

cindy-peng added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 13, 2024

cindy-peng requested review from a team as code owners April 13, 2024 01:23

cindy-peng requested a review from gkevinzheng April 13, 2024 01:23

product-auto-label bot added size: l Pull request size is large. api: logging Issues related to the googleapis/nodejs-logging API. labels Apr 13, 2024

cindy-peng removed the request for review from gkevinzheng April 13, 2024 01:24

cindy-peng and others added 4 commits April 12, 2024 18:43

remove only mocha test keywords

e5da43f

Fix lint errors

809b940

fix lint issues

73de242

🦉 Updates from OwlBot post-processor

c254669

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Apr 16, 2024

cindy-peng removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 16, 2024

cindy-peng and others added 2 commits April 16, 2024 16:03

Add span end method in system tests

1da8791

Merge branch 'main' into cindy/spanidfix

94e5f47

cindy-peng self-assigned this Apr 16, 2024

cindy-peng and others added 5 commits April 16, 2024 16:07

fix gts check

82a89bb

🦉 Updates from OwlBot post-processor

9977f11

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Merge branch 'cindy/spanidfix' of https://github.com/googleapis/nodej…

d2af395

…s-logging into cindy/spanidfix

🦉 Updates from OwlBot post-processor

cfdff26

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Merge branch 'cindy/spanidfix' of https://github.com/googleapis/nodej…

5d926fd

…s-logging into cindy/spanidfix

gkevinzheng reviewed Apr 24, 2024

View reviewed changes

daniel-sanche reviewed Apr 24, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

daniel-sanche reviewed Apr 24, 2024

View reviewed changes

cindy-peng added 2 commits April 25, 2024 11:33

Remove comments

20e93c9

move otel packages to peer dependencies

24be272

product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Apr 25, 2024

cindy-peng added 2 commits April 25, 2024 11:46

move otel packages to peer dependencies

2620b16

fix system test

e63c546

daniel-sanche approved these changes Apr 25, 2024

View reviewed changes

gkevinzheng approved these changes Apr 25, 2024

View reviewed changes

cindy-peng and others added 3 commits April 26, 2024 10:23

Merge branch 'main' into cindy/spanidfix

7620bb5

add bitmask for tracesampled flag

91c3d6e

fix pack-n-play testing

7260c2b

cindy-peng added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 29, 2024

cindy-peng and others added 3 commits April 29, 2024 15:40

revert pack-n-play sample configuration

dfe4202

Make otel api as dependency due to low npm version in kokoro infra

567b953

🦉 Updates from OwlBot post-processor

3342bb7

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

aabmass approved these changes May 7, 2024

View reviewed changes

fix comments to return value directy

f4dfbff

product-auto-label bot added the stale: old Pull request is old and needs attention. label May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Open telemetry integration and span Id fix for nodejs logging library #1497

feat: Open telemetry integration and span Id fix for nodejs logging library #1497

cindy-peng commented Apr 13, 2024 •

edited

gkevinzheng left a comment

gkevinzheng Apr 24, 2024

cindy-peng Apr 25, 2024

cindy-peng Apr 30, 2024

cindy-peng May 7, 2024

daniel-sanche left a comment

cindy-peng commented Apr 25, 2024

gkevinzheng left a comment

cindy-peng commented Apr 29, 2024

aabmass left a comment

aabmass May 7, 2024

cindy-peng May 7, 2024

feat: Open telemetry integration and span Id fix for nodejs logging library #1497

Are you sure you want to change the base?

feat: Open telemetry integration and span Id fix for nodejs logging library #1497

Conversation

cindy-peng commented Apr 13, 2024 • edited

gkevinzheng left a comment

Choose a reason for hiding this comment

gkevinzheng Apr 24, 2024

Choose a reason for hiding this comment

cindy-peng Apr 25, 2024

Choose a reason for hiding this comment

cindy-peng Apr 30, 2024

Choose a reason for hiding this comment

cindy-peng May 7, 2024

Choose a reason for hiding this comment

daniel-sanche left a comment

Choose a reason for hiding this comment

cindy-peng commented Apr 25, 2024

gkevinzheng left a comment

Choose a reason for hiding this comment

cindy-peng commented Apr 29, 2024

aabmass left a comment

Choose a reason for hiding this comment

aabmass May 7, 2024

Choose a reason for hiding this comment

cindy-peng May 7, 2024

Choose a reason for hiding this comment

cindy-peng commented Apr 13, 2024 •

edited