Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add faraday instrumentation adapter #67 #148

Merged
merged 35 commits into from
Jan 9, 2020
Merged

Add faraday instrumentation adapter #67 #148

merged 35 commits into from
Jan 9, 2020

Conversation

duonoid
Copy link
Contributor

@duonoid duonoid commented Nov 11, 2019 •
edited
Loading

Overview

Adds faraday instrumentation adapter.

Details

  • faraday adapter exists in its own gem
  • Adds a runnable example for demonstration purposes and testing

Related

Don Morrison and others added 4 commits November 11, 2019 13:57
@duonoid
Add API OpenTelemetry::Adapter
079d98d 
* for access to global config, e.g., OpenTelemetry::Adapter.tracer
@duonoid
Add faraday instrumentation adapter Gemfile/gemspec
5ac7d4e
@duonoid
Add faraday instrumentation adapter
3f7f6b0
@duonoid
Add faraday example with docker-compose service
ed31c85 
* e.g.,
  $ docker-compose run ex-adapter-faraday
  $ ruby faraday.rb

Expected: console output of SpanData
@duonoid duonoid mentioned this pull request Nov 11, 2019
Merged
mwear
mwear reviewed Nov 12, 2019
View reviewed changes
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks like it's on the right path. I have a handful of comments that follow. In addition to those, we'll need to consider testing and how that would look.

adapters/faraday/lib/opentelemetry/adapters/faraday.rb Outdated Show resolved Hide resolved
adapters/faraday/lib/opentelemetry/adapters/faraday/middlewares/tracer_middleware.rb Show resolved Hide resolved
adapters/faraday/lib/opentelemetry/adapters/faraday/middlewares/tracer_middleware.rb Show resolved Hide resolved
@duonoid
Avoid hard-dependency on faraday
22ccf4f 
* want to be able to ship more instrumentation than a user needs,
  let the auto-installer determine what is applicable
@elskwid elskwid mentioned this pull request Nov 13, 2019
Closed
@duonoid
Fix undefined method `version' for nil:NilClass
7562579 
* when 'faraday' isn't loaded/required already

e.g.,
irb> require 'opentelemetry/adapters/faraday'
irb> OpenTelemetry::Adapters::Faraday.install
@duonoid
Doc: reminder to handle common use case
6987351
@duonoid
Add explicit 'require' statements in faraday/adapter
92ba1bc 
* will only be called after Adapters::Faraday.install
* reduces burden on caller
@duonoid
Use in_span(attributes:) instead of set_attribute
60d41d6 
* for performance, since span.set_attribute involves a mutex, etc.
@duonoid
Change SDK::Trace::TracerFactory to subclass API's TracerFactory
33f28c0 
* to allow access to #http_text_format
@duonoid
Add context propagation
937cf6c
@duonoid
Fix occasional test failure in sdk batch_span_processor_test
d42721b 
* RE: "failures on heavily loaded CI server" -- increase delta
  experimentally, by 10x
@duonoid
Merge remote-tracking branch 'upstream/master' into dd/wip--faraday-i…
0e0d69f 
…nstrumentation--67

# By David Davis (1) and others
# Via GitHub
* upstream/master:
  Update versions for v0.2.0 release (#152)
  SDK: integration tests (#143)
  Add http client/server example (#131)

# Conflicts:
#	docker-compose.yml
@duonoid
Don't extract context, just inject
814e330 
hints:
* for outbound requests:
  inject current span context into request headers
* for inbound requests:
  extract remote span context from request headers,
  start a new span with parent context (using the extracted
  context)
@duonoid
Load tracer_version lazily
4ee20b1 
* TRACER_VERSION can get locked to empty string if faraday isn't required yet
@duonoid
Allow auto-installer to inject a tracer
915b052 
* allow (auto) installer to bind tracer to instrumentation
* config[:tracer] will override default_tracer
@duonoid
Merge remote-tracking branch 'upstream/master' into faraday-instrumen…
89e6453 
…tation--67

...

# Conflicts:
#	sdk/test/opentelemetry/sdk/trace/export/batch_span_processor_test.rb
@duonoid duonoid mentioned this pull request Nov 18, 2019
Closed
@duonoid
Copy link
Contributor Author

duonoid commented Nov 18, 2019

I've extracted the API changes to #154.

@duonoid duonoid mentioned this pull request Nov 19, 2019
Closed
@duonoid
Copy link
Contributor Author

duonoid commented Nov 19, 2019

we'll need to consider testing and how that would look

I have noted this point in issue #155.

@duonoid duonoid marked this pull request as ready for review November 19, 2019 17:33
@duonoid duonoid requested a review from bai as a code owner November 19, 2019 17:33
@duonoid
Add test_helper and first test
21c8ada 
* note: adapter.install effects aren't localized, so test order
  matters

Update tests to avoid passing name, version to .install

* name and version are now determined internally and shouldn't usually
  be passed in explicitly
@duonoid
Add appraisals for running tests against multiple versions
eb243a9 
example:
$ bundle exec appraisal install
$ bundle exec appraisal rake test
@duonoid
Add Circle CI config
cfcdf5f
@duonoid
Add TracerMiddleware test
dd680bb
@duonoid
Add Adapter test
5808423
@duonoid
Copy link
Contributor Author

duonoid commented Dec 6, 2019

I have merged in the testing work mentioned in #155, now that the "shape" of the API is a little more solid (per sig feedback, etc.).

mwear
mwear approved these changes Dec 12, 2019
View reviewed changes
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hard work. Given what we know today, this looks like a very solid preliminary blueprint for how we want to package instrumentation adapters for OTel. I'm approving this and will leave it open in case @fbogsany has any last requests.

fbogsany
fbogsany approved these changes Dec 18, 2019
View reviewed changes
Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. We should figure out how to address the "disable for span reporting connection" issue and document it for authors of exporters.

@duonoid
Allow for easier overriding of TracerMiddleware for per-connection rules
bc9e879 
* Use case: allow per-connection enabling/disabling of span reporting,
  possibly based on URL rules
@duonoid
Fix failing circleci builds
73f6086 
* most likely failing due to bundler-2.1.x
* see bundler issue #7489
@duonoid
Fix failing circleci - jruby
fd66e7d 
* circleci build is failing after jruby:9.2.9-jre
* theory: jruby includes (vendored) its own bundler version
@duonoid
Appraisal: add faraday-1.0.x
4098dd4
@duonoid
Copy link
Contributor Author

duonoid commented Jan 6, 2020

I saw that faraday-1.0.0 was released, and have added an Appraisal for it.

Also, there seems to be an issue with circleci + bundler-2.1.x (including 2.1.4) + appraisal (2.2.0), which is solved locally by using bundler-2.0.x and jruby-2.8.x (circleci config).

@mwear
Copy link
Member

mwear commented Jan 9, 2020

👋 @duonoid. I had to integrate the changes from master before merging this PR (which I was just about to do). We now test against Ruby 2.7 and those tests are failing due to not being able to find a compatible version of bundler: https://circleci.com/gh/mutations/opentelemetry-ruby/1498?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link. Could you take a look?

@duonoid
Copy link
Contributor Author

duonoid commented Jan 9, 2020

@mwear I had previously been able to work around the bundler+appraisal issues that I saw by uninstalling other versions of bundler and using, e.g., 2.0.x. It seems that ruby-2.7 is now including bundler-2.1.2 as a default gem, which cannot be uninstalled because it is a default gem

thoughtbot/appraisal#162 mentions a similar issue

For an immediate workaround, I could try removing Appraisal and just use rake test.

@duonoid
circleci: Remove appraisal support, use latest bundler
36cfd8a 
* TODO: re-enable appraisal support once appraisal issue #162 is solved
@mwear
Copy link
Member

mwear commented Jan 9, 2020

Does this mean that we are only testing against one version of Faraday now? If so, I'm wondering how hard it would be to disable appraisal for Ruby 2.7 only. If it's more effort than it should be, then I'd settle for this and an issue to reinstate appraisal once a fix has been found.

@duonoid
circleci: Workaround failing appraisal+bundler(2.1.x)+ruby-2.7 issue
dc05f82 
* don't run bundle exec appraisal rake test under ruby-2.7
* NOTE: build was failing under jruby:latest (9.3.x) as well
@duonoid
Copy link
Contributor Author

duonoid commented Jan 9, 2020

@mwear I have made some circle ci config changes to exclude 'appraisals' from running under ruby-2.7 for the time-being.

@mwear
Copy link
Member

mwear commented Jan 9, 2020

🎉 Thanks for all your hard work @duonoid!

@mwear mwear merged commit 2e083fd into open-telemetry:master Jan 9, 2020
This was referenced Jan 14, 2020
Merged
Merged
@grantbdev grantbdev mentioned this pull request Jan 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benedictfischer09 benedictfischer09 benedictfischer09 left review comments

@mwear mwear mwear approved these changes

@fbogsany fbogsany fbogsany approved these changes

@bai bai Awaiting requested review from bai

@dazuma dazuma Awaiting requested review from dazuma

@elskwid elskwid Awaiting requested review from elskwid

@luvtechno luvtechno Awaiting requested review from luvtechno

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@duonoid @mwear @elskwid @fbogsany @benedictfischer09