-
Notifications
You must be signed in to change notification settings - Fork 237
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: instrument google apis core #631
Conversation
d22760a
to
bc0e9cf
Compare
module GoogleApisCore | ||
module Patches | ||
# Module to prepend to ::Google::Apis::Core::HttpCommand for instrumentation | ||
module HttpCommand |
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.
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.
I would strongly prefer just to edit google-apis-core itself to provide whatever explicit hooks are needed. Or perhaps even to build the instrumentation directly into google-apis-core, the way that it currently has opencensus built in. So we can avoid monkey patches. (I effectively own google-apis-core myself, so I can fast-track any such work and release.)
[license-github]: https://github.com/open-telemetry/opentelemetry-ruby/blob/main/LICENSE | ||
[ruby-sig]: https://github.com/open-telemetry/community#ruby-sig | ||
[community-meetings]: https://github.com/open-telemetry/community#community-meetings | ||
[ruby-gitter]: https://gitter.im/open-telemetry/opentelemetry-ruby |
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.
Do we need to fix the generator script to refer to GitHub Discussions instead of Gitter? I thought I'd fixed every reference 😞
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.
I used the generator before your change went in :), I'll remove this from the PR.
module HttpCommand | ||
def opencensus_begin_span | ||
return if @opentelemetry_tracing_span | ||
return unless OpenTelemetry::Trace.current_span.context.trace_flags.sampled? |
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.
I know we did this in the Shopify instrumentation, to match the check in the method we're monkey-patching, but I don't think that's really in the spirit of OpenTelemetry. If we're trying to avoid overhead from instrumentation when we're not recording, the supported way to do that is:
return unless OpenTelemetry::Trace.current_span.context.trace_flags.sampled? | |
return unless OpenTelemetry::Trace.current_span.recording? |
@dazuma do you have an opinion on this?
.../google_apis_core/lib/opentelemetry/instrumentation/google_apis_core/patches/http_command.rb
Outdated
Show resolved
Hide resolved
|
||
The `opentelemetry-instrumentation-google_apis_core` gem source is [on github][repo-github], along with related gems including `opentelemetry-api` and `opentelemetry-sdk`. | ||
|
||
The OpenTelemetry Ruby gems are maintained by the OpenTelemetry-Ruby special interest group (SIG). You can get involved by joining us on our [gitter channel][ruby-gitter] or attending our weekly meeting. See the [meeting calendar][community-meetings] for dates and times. For more information on this and other language SIGs, see the OpenTelemetry [community page][ruby-sig]. |
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.
on our [gitter channel][ruby-gitter]
This should refer to GitHub Discussions instead.
Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
Closing as this will be added directly to the google apis core gem. |
Google apis core gem PR googleapis/google-api-ruby-client#3414 |
Instruments https://github.com/googleapis/google-api-ruby-client/tree/master/google-apis-core
Closes: open-telemetry/opentelemetry-ruby-contrib#25