fix: added support for Opentelemetry 0.18 #1234

weyert · 2021-03-17T22:58:24Z

Introduces support for Opentelemetry 0.18 API which implements the
Tracing 1.0 specification.

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 #1233 🦕

Introduces support for Opentelemetry 0.18 API which implements the Tracing 1.0 specification.

google-cla · 2021-03-17T22:58:27Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

codecov · 2021-03-17T22:59:53Z

Codecov Report

Merging #1234 (4f360e4) into master (e85b34c) will increase coverage by 0.04%.
The diff coverage is 99.03%.

@@            Coverage Diff             @@
##           master    #1234      +/-   ##
==========================================
+ Coverage   97.80%   97.84%   +0.04%     
==========================================
  Files          26       26              
  Lines       12642    12700      +58     
  Branches      611      607       -4     
==========================================
+ Hits        12364    12426      +62     
+ Misses        273      268       -5     
- Partials        5        6       +1

Impacted Files	Coverage Δ
src/publisher/index.ts	`97.40% <96.66%> (-0.22%)`	⬇️
src/opentelemetry-tracing.ts	`100.00% <100.00%> (ø)`
src/subscriber.ts	`99.82% <100.00%> (+0.94%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e85b34c...4f360e4. Read the comment docs.

google-cla · 2021-03-17T23:01:17Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

weyert · 2021-03-17T23:02:24Z

@googlebot I consent

google-cla · 2021-03-17T23:02:28Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

tapico-weyert · 2021-03-17T23:03:40Z

@googlebot I consent

src/opentelemetry-tracing.ts

…ateSpan as function

package.json

aabmass

Question to the maintainers here, can we remove enableOpenTelemetryTracing option altogether?

Enabling instrumentation in OpenTelemetry should be done by

User adding a dependency on the SDK (instead of just the API) @opentelemetry/tracing (which is why @weyert has moved it out of dependencies)
User configuring the global tracer provider to the SDK TracerProvider as is done here:

nodejs-pubsub/samples/openTelemetryTracing.js

Line 66 in 8838288

provider.register();

An alternative is the user can pass a tracer provider instance as an option, otherwise we get the global one.

If the user doesn't take those steps, the @opentelemetry/api package will give no-op implementations that don't do anything. It should be implicit

src/opentelemetry-tracing.ts

aabmass · 2021-03-18T17:12:45Z

test/publisher/index.ts

@@ -223,7 +192,15 @@ describe('Publisher', () => {
      opentelemetry.trace.setGlobalTracerProvider(provider);

      tracingPublisher.publish(buffer);
-      assert.ok(exporter.getFinishedSpans());
+      const spans = exporter.getFinishedSpans();


Nice this looks better

yes, the problem is that it wasn't checking if it had a span so it always was successful (even now in master) if I change this to check the length >= 1 it will fail.

Only I am not well versed in this library or its tests to see were the issue is. It does like that _constructSpan always exits early due the first if-statement.

So its still not creating a span with the current code?

Yeah, I tried it a minute ago and it looks like it's still not getting any spans. I thought maybe it's because you have to add a tracer, but that doesn't seem to have changed it. I don't know much about OT either :)

Slightly unrelated: assert.equal shows up as deprecated for me, and it suggests assert.strictEqual.

I'll take a look

@weyert I'll open a PR to your fork. I think my original suggestions were too lofty, I want to sync up with the pubsub folks before changing things too much.

test/subscriber.ts

Small refactor to ensure no new tracer gets created for each call to the `createSpan`-functio by initiating the `libraryTracer`-variable

feywind · 2021-03-18T18:57:22Z

Thanks for all the code and comments, I appreciate it... I haven't really known what to do with the OT support since the APIs changed. It was an outside contribution, so none of us know that API too well. So to answer @aabmass 's comment, I'm open to guidance on where to take it over time.

Busy day today, I'll look at this in a bit! :)

weyert · 2021-03-19T13:20:08Z

Thanks for all the code and comments, I appreciate it... I haven't really known what to do with the OT support since the APIs changed. It was an outside contribution, so none of us know that API too well. So to answer @aabmass 's comment, I'm open to guidance on where to take it over time.

Busy day today, I'll look at this in a bit! :)

Yes, the API shouldn't really happen anymore as the API is 1.0.0 now and it shouldn't change much anymore and I think only backward compatible changes are allowed and 1.0.0 will be supported for a few years. The opentelemetry-js team is planning to release 1.0.0 rc of the SDK and API soon but not much has changed between 0.18 and 1.0.0-rc. I think migrating to 0.18 is a good start in the right direction.

Currently, I have upgraded to 0.18 and tried to remove the dependency to the tracing SDK package which isn't met to be used by libraries. Only during my changes I am have found a broken test which even occurs in the master-branch when improving the assert.ok to actually check that a span is being created. Until now it looks it only checks it was a thruthly value

aabmass · 2021-03-22T19:24:28Z

@weyert the test failure looks to be related to globals and things running before this test. If I only run that case with npm run test -- -g 'export created spans' it passes. Might be related to the change I requested for the module level tracer.

I think depending when @feywind wants to make more changes, we should change things a bit. If we alter the configuration here and remove enableOpentelemetryTracing, it is a breaking change and would require a major release.

So for now @weyert, lets maybe leave the config in and just upgrade the library to 0.18?

weyert · 2021-03-22T19:32:42Z

@weyert the test failure looks to be related to globals and things running before this test. If I only run that case with npm run test -- -g 'export created spans' it passes. Might be related to the change I requested for the module level tracer.

Thanks for trying this out! I didn't think about trying this out!

I think depending when @feywind wants to make more changes, we should change things a bit. If we alter the configuration here and remove enableOpentelemetryTracing, it is a breaking change and would require a major release.

So for now @weyert, lets maybe leave the config in and just upgrade the library to 0.18?

Sounds good, so the change to startSpan and revert back to using the class?

Sorry, I haven't had a change yet to look into it today.

aabmass · 2021-03-22T22:05:38Z

Sounds good, so the change to startSpan and revert back to using the class?

Hmm you mentioned it was broken before this PR too so maybe that won't fix it :/ but its worth a try

feywind · 2021-04-09T23:14:47Z

We talked over the package.json thing, I think that's probably not going to change for now, but we might do something to make it more abstracted/standardized. I'd like to do it separately if we can get a clean build on this, though.

weyert · 2021-04-09T23:38:37Z

We talked over the package.json thing, I think that's probably not going to change for now, but we might do something to make it more abstracted/standardized. I'd like to do it separately if we can get a clean build on this, though.

That makes totally sense! :)

weyert · 2021-04-09T23:40:42Z

package.json

@@ -38,7 +38,7 @@
    "clean": "gts clean",
    "compile": "tsc -p . && cp -r protos build/",
    "compile-protos": "compileProtos src",
-    "prepare": "npm run compile-protos && npm run compile && rm ./build/package.json",
+    "prepare": "npm run compile-protos && npm run compile",


I think you would need to revert the change from require to import approach for loading package.json. I don't think removing this is enough

feywind · 2021-04-09T23:45:02Z

I'm a little bit confused, this branch now works and builds for me just fine locally. Actually removing the package.json in build broke some things for me. It's getting late here though, I'm going to look more next week.

weyert · 2021-04-10T00:29:08Z

I'm a little bit confused, this branch now works and builds for me just fine locally. Actually removing the package.json in build broke some things for me. It's getting late here though, I'm going to look more next week.

Most important that it works now :D Yeah confusing indeed.

weyert · 2021-04-13T15:35:56Z

Any news?

feywind · 2021-04-13T18:29:31Z

Not yet, sorry. I'm going to just retry it again for good measure, in case it was a (long lasting) transient error. I don't think anything is wrong with the code at this point.

feywind · 2021-04-13T22:32:14Z

@weyert On my local machine, I did manage to get this part of the process to fail, but it was a different failure. I'm not sure why package.json would be required in files now, when it wasn't before. But I still don't get the Kokoro error, and I'm starting to wonder if someone tweaked settings somewhere. I'm asking others to see if they have any ideas.

weyert · 2021-04-13T23:03:54Z

@weyert On my local machine, I did manage to get this part of the process to fail, but it was a different failure. I'm not sure why package.json would be required in files now, when it wasn't before. But I still don't get the Kokoro error, and I'm starting to wonder if someone tweaked settings somewhere. I'm asking others to see if they have any ideas.

If you run it locally, what kind of output does the issues npm pack-command give you? The problem I had last week that something caused it not to bundle all files, see the problematic output below. If you look closely it's not including the whole build-directory only build/package.json is included. That's why it can't find the type definitions. Does this happen to you too?

npm notice 
npm notice 📦  @google-cloud/pubsub@2.10.0
npm notice === Tarball Contents === 
npm notice 11.4kB LICENSE           
npm notice 3.6kB  build/package.json
npm notice 3.2kB  package.json      
npm notice 57.5kB CHANGELOG.md      
npm notice 20.2kB README.md         
npm notice === Tarball Details === 
npm notice name:          @google-cloud/pubsub                    
npm notice version:       2.10.0                                  
npm notice filename:      google-cloud-pubsub-2.10.0.tgz          
npm notice package size:  22.0 kB                                 
npm notice unpacked size: 95.8 kB                                 
npm notice shasum:        3c77e1e46cfcbccb3cbd2de732d81a6fc5ff7823
npm notice integrity:     sha512-3H4qqXZWOSogu[...]6ERB1uWfewv2g==
npm notice total files:   5                                       
npm notice 
google-cloud-pubsub-2.10.0.tgz

weyert · 2021-04-13T23:07:58Z

What we could try @feywind is to revert basically this change: 0c2c76f and see what Kokoro things of it all.

This reverts commit 0c2c76f.

This reverts commit 14de625.

feywind · 2021-04-14T19:25:49Z

!! 🎉
@weyert @aabmass Thank you both for all the work on this.

weyert · 2021-04-15T03:45:51Z

Awesome! Thank you for merging :D

fix: added support for Opentelemetry 0.18

cf0217d

Introduces support for Opentelemetry 0.18 API which implements the Tracing 1.0 specification.

weyert requested review from a team as code owners March 17, 2021 22:58

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Mar 17, 2021

product-auto-label bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Mar 17, 2021

style: remove unused import declaration

c01a210

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 17, 2021

aabmass reviewed Mar 17, 2021

View reviewed changes

src/opentelemetry-tracing.ts Outdated Show resolved Hide resolved

tapico-weyert added 4 commits March 17, 2021 23:54

refactor: removed the OpenTelemetryTracer-class and just expose cre…

0de436e

…ateSpan as function

test: updated the span related tests

c36c9f5

test: updated the tests

e790c23

test: updated the tests

d96d2d9

aabmass reviewed Mar 18, 2021

View reviewed changes

package.json Show resolved Hide resolved

aabmass reviewed Mar 18, 2021

View reviewed changes

refactor: move the tracer out of the createSpan-function

92b1a4c

Small refactor to ensure no new tracer gets created for each call to the `createSpan`-functio by initiating the `libraryTracer`-variable

Merge branch 'master' into remove-tracing-dependency

b884b21

feywind mentioned this pull request Mar 19, 2021

fix(deps): update opentelemetry monorepo #1231

Closed

1 task

Merge branch 'master' into remove-tracing-dependency

8b8d97e

build: put back package.json in the build

9339baf

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 9, 2021

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

weyert commented Apr 9, 2021

View reviewed changes

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2021

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

build: explicitly add package.json to the npm pack build

14de625

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2021

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

feywind added 2 commits April 14, 2021 14:39

fix: revert "fix: change the way package.json is being imported"

253b56b

This reverts commit 0c2c76f.

fix: revert "build: explicitly add package.json to the npm pack build"

4db9ba5

This reverts commit 14de625.

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 14, 2021

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

Merge branch 'master' into remove-tracing-dependency

4f360e4

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 14, 2021

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

feywind merged commit aedc36c into googleapis:master Apr 14, 2021

weyert deleted the remove-tracing-dependency branch April 15, 2021 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: added support for Opentelemetry 0.18 #1234

fix: added support for Opentelemetry 0.18 #1234

weyert commented Mar 17, 2021

google-cla bot commented Mar 17, 2021

codecov bot commented Mar 17, 2021 •

edited

google-cla bot commented Mar 17, 2021

weyert commented Mar 17, 2021

google-cla bot commented Mar 17, 2021

tapico-weyert commented Mar 17, 2021

aabmass left a comment

aabmass Mar 18, 2021

weyert Mar 18, 2021

aabmass Mar 19, 2021 •

edited

feywind Mar 19, 2021

aabmass Mar 19, 2021

aabmass Mar 19, 2021

feywind commented Mar 18, 2021

weyert commented Mar 19, 2021

aabmass commented Mar 22, 2021

weyert commented Mar 22, 2021

aabmass commented Mar 22, 2021

feywind commented Apr 9, 2021

weyert commented Apr 9, 2021

weyert Apr 9, 2021

feywind commented Apr 9, 2021

weyert commented Apr 10, 2021

weyert commented Apr 13, 2021

feywind commented Apr 13, 2021

feywind commented Apr 13, 2021

weyert commented Apr 13, 2021

weyert commented Apr 13, 2021

feywind commented Apr 14, 2021

weyert commented Apr 15, 2021

fix: added support for Opentelemetry 0.18 #1234

fix: added support for Opentelemetry 0.18 #1234

Conversation

weyert commented Mar 17, 2021

google-cla bot commented Mar 17, 2021

codecov bot commented Mar 17, 2021 • edited

Codecov Report

google-cla bot commented Mar 17, 2021

weyert commented Mar 17, 2021

google-cla bot commented Mar 17, 2021

tapico-weyert commented Mar 17, 2021

aabmass left a comment

Choose a reason for hiding this comment

aabmass Mar 18, 2021

Choose a reason for hiding this comment

weyert Mar 18, 2021

Choose a reason for hiding this comment

aabmass Mar 19, 2021 • edited

Choose a reason for hiding this comment

feywind Mar 19, 2021

Choose a reason for hiding this comment

aabmass Mar 19, 2021

Choose a reason for hiding this comment

aabmass Mar 19, 2021

Choose a reason for hiding this comment

feywind commented Mar 18, 2021

weyert commented Mar 19, 2021

aabmass commented Mar 22, 2021

weyert commented Mar 22, 2021

aabmass commented Mar 22, 2021

feywind commented Apr 9, 2021

weyert commented Apr 9, 2021

weyert Apr 9, 2021

Choose a reason for hiding this comment

feywind commented Apr 9, 2021

weyert commented Apr 10, 2021

weyert commented Apr 13, 2021

feywind commented Apr 13, 2021

feywind commented Apr 13, 2021

weyert commented Apr 13, 2021

weyert commented Apr 13, 2021

feywind commented Apr 14, 2021

weyert commented Apr 15, 2021

codecov bot commented Mar 17, 2021 •

edited

aabmass Mar 19, 2021 •

edited