-
Notifications
You must be signed in to change notification settings - Fork 870
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 opentelemetry instrumentation #3414
Conversation
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
'peer.service' => url.host.to_s | ||
} | ||
|
||
@opentelemetry_span = Google::Apis::Core::OpenTelemetry.instance.tracer.start_span(url.host.to_s, attributes: attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, what's the best practice for naming spans? This just provides the service name. Should we include the method being called?
#3185 (comment)
That's a good question as this isn't always exactly an HTTP span, if the consumers of this instrumentation also include the http client instrumentation these spans will be wrapping the HTTP calls.
For our internal instrumentation we've been using url.host.to_s
as the span name.
b1634b8
to
9683dfa
Compare
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @dazuma, there was a bit of a delay getting approval for the CLA.
require 'opentelemetry-api' | ||
|
||
module Google | ||
module Apis | ||
module Core | ||
class OpenTelemetry < OpenTelemetry::Instrumentation::Base | ||
install { true } | ||
present { true } | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the addition of the dynamic upgradeable tracer do we still want to make use of this?
I've removed this, I think the best version of this remove any dependencies to opentelemetry that is not the api gem.
if block_given? | ||
yield result, nil | ||
url_host = url.host.to_s | ||
OTEL_TRACER.in_span(url_host) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have instrumentation for the underlying http lib (http client), so I think it makes sense to not duplicate all the HTTP attributes on this span.
This span also serves the purpose of grouping retired network calls under a common parent span.
What's the status of this work? Asking because I am very much interested in putting this to use. Also, is there additional code somewhere that would set attributes like is done for the aws_sdk in https://github.com/open-telemetry/opentelemetry-ruby/blob/f0593e7d97c62b10862c382d14f533dbc49be580/instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb#L23-L28 ? |
Hi @robertlaurin , I'm so sorry we completely lost track of this. Is it still updated? Would you like to take a look and update the PR accordingly? |
Hi @robertlaurin |
Provides instrumentation for https://github.com/open-telemetry/opentelemetry-ruby
Original draft PR feedback here #3185