[WIP] feat: add opentelemetry instrumentation #3185

robertlaurin · 2021-03-08T23:28:16Z

No description provided.

google-cla · 2021-03-08T23:28:31Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

robertlaurin

Hey @dazuma, here's rough draft here to get the discussion/review started.

robertlaurin · 2021-03-08T23:29:06Z

google-apis-core/lib/google/apis/core/http_command.rb

@@ -30,10 +30,10 @@ class HttpCommand
        RETRIABLE_ERRORS = [Google::Apis::ServerError, Google::Apis::RateLimitError, Google::Apis::TransmissionError]

        begin
-          require 'opencensus'
-          OPENCENSUS_AVAILABLE = true


Did you want to maintain support for opencensus in addition to opentelemetry?

Uhh, yeah, as much as I don't want to support OpenCensus here anymore, I also don't think we should rip it out unceremoniously. We should rather add a deprecation warning for a time. Can we use the logic of: try requiring opentelemetry and if that works enable it, otherwise try requiring opencensus and if that works enable it, otherwise disable both? So you can just leave the opencensus_{begin,end}_span methods in place, and add new ones for OpenTelemetry.

robertlaurin · 2021-03-08T23:30:04Z

google-apis-core/lib/google/apis/core/instrumentation.rb

+module Google
+  module Apis
+    module Core
+      class Instrumentation < OpenTelemetry::Instrumentation::Base


I included the instrumentation base as it surfaces configuration in a consistent way with other instrumented gems.

robertlaurin · 2021-03-08T23:31:14Z

google-apis-core/spec/google/apis/core/http_command_spec.rb

-        expect(events[0].uncompressed_size).to eql 0
-        expect(events[1].type).to eql :RECEIVED
-        expect(events[1].uncompressed_size).to eql 11
+      span_double = instance_double(OpenTelemetry::Trace::Span)


I don't typically reach for doubles, but I was apprehensive towards including opentelemetry-sdk so we could use the in memory exporter to assert the generated spans.

Why were you apprehensive? Perfectly happy including the SDK gem as a development dependency (not a runtime dependency) so it's available for tests, if it will make this cleaner.

Why were you apprehensive?

I just didn't want to assume that you would want the SDK included so I opted to present a quick alternative first so I could get your feedback. I will change this to use the SDK as I address the rest of your feedback.

dazuma · 2021-03-11T00:14:18Z

google-apis-core/lib/google/apis/core/http_command.rb

@@ -30,10 +30,10 @@ class HttpCommand
        RETRIABLE_ERRORS = [Google::Apis::ServerError, Google::Apis::RateLimitError, Google::Apis::TransmissionError]

        begin
-          require 'opencensus'
-          OPENCENSUS_AVAILABLE = true


Uhh, yeah, as much as I don't want to support OpenCensus here anymore, I also don't think we should rip it out unceremoniously. We should rather add a deprecation warning for a time. Can we use the logic of: try requiring opentelemetry and if that works enable it, otherwise try requiring opencensus and if that works enable it, otherwise disable both? So you can just leave the opencensus_{begin,end}_span methods in place, and add new ones for OpenTelemetry.

dazuma · 2021-03-11T00:33:08Z

google-apis-core/lib/google/apis/core/http_command.rb

+            'http.host' => url.host.to_s,
+            'http.method' => method.to_s,
+            'http.target' => url.path.to_s,
+            'peer.service' => 'google'


I don't think most google backends are actually emitting spans or setting service.name so it may not matter that much... But from my understanding of the semantics of this attribute, I suggest using url.host.to_s which will return the "official" ID of the service being called.

dazuma · 2021-03-11T00:39:10Z

google-apis-core/lib/google/apis/core/http_command.rb

          # the entire request to fail.
-          logger.debug { sprintf('Error opening OpenCensus span: %s', e) }
+          logger.debug { sprintf('Error opening OpenTelemetry span: %s', e) }


I know the opencensus code used sprintf. That was unfortunate and I should have changed it. Could you just use normal string interpolation for this?

dazuma · 2021-03-11T00:40:07Z

google-apis-core/lib/google/apis/core/http_command.rb

+            'peer.service' => 'google'
+          }
+
+          @opentelemetry_span = Google::Apis::Core::Instrumentation.instance.tracer.start_span(url.host.to_s, attributes: attributes)


In general, what's the best practice for naming spans? This just provides the service name. Should we include the method being called?

dazuma · 2021-03-11T00:44:17Z

google-apis-core/spec/google/apis/core/http_command_spec.rb

-        expect(events[0].uncompressed_size).to eql 0
-        expect(events[1].type).to eql :RECEIVED
-        expect(events[1].uncompressed_size).to eql 11
+      span_double = instance_double(OpenTelemetry::Trace::Span)


Why were you apprehensive? Perfectly happy including the SDK gem as a development dependency (not a runtime dependency) so it's available for tests, if it will make this cleaner.

robertlaurin · 2021-03-23T15:22:34Z

Accidentally did a bad rebase the close the PR :/

Re-opened -> #3414

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

robertlaurin commented Mar 8, 2021

View reviewed changes

fbogsany mentioned this pull request Jun 14, 2022

Google API client instrumentation open-telemetry/opentelemetry-ruby-contrib#25

Closed

dazuma reviewed Mar 11, 2021

View reviewed changes

dazuma added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 11, 2021

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

robertlaurin closed this Mar 23, 2021

robertlaurin force-pushed the feat-add-opentelemetry branch from fcb8442 to d2f2abb Compare March 23, 2021 15:19

robertlaurin mentioned this pull request Mar 23, 2021

feat: add opentelemetry instrumentation #3414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: add opentelemetry instrumentation #3185

[WIP] feat: add opentelemetry instrumentation #3185

robertlaurin commented Mar 8, 2021

google-cla bot commented Mar 8, 2021

robertlaurin left a comment

robertlaurin Mar 8, 2021

dazuma Mar 11, 2021

robertlaurin Mar 8, 2021

robertlaurin Mar 8, 2021

dazuma Mar 11, 2021

robertlaurin Mar 17, 2021 •

edited

Loading

dazuma Mar 11, 2021

dazuma Mar 11, 2021

dazuma Mar 11, 2021

dazuma Mar 11, 2021

dazuma Mar 11, 2021

robertlaurin commented Mar 23, 2021

[WIP] feat: add opentelemetry instrumentation #3185

[WIP] feat: add opentelemetry instrumentation #3185

Conversation

robertlaurin commented Mar 8, 2021

google-cla bot commented Mar 8, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

robertlaurin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertlaurin Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertlaurin commented Mar 23, 2021

robertlaurin Mar 17, 2021 •

edited

Loading