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.

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

Fix adapter examples and simplify setup #177

Merged
merged 4 commits into from
Jan 27, 2020

Conversation

mwear
Copy link
Member

@mwear mwear commented Jan 25, 2020

This PR fixes the Faraday and Sinatra examples and updates them to use the instrumentation auto-installer and SDK configuration improvements that landed last week.

It also updates the Adapter subclasses to check for presence using constants in the target library rather than using Gem.loaded_specs. The reason is that a gemspec can be loaded, but the target library might not be required. The instrumentation installer handles exceptions gracefully, but will log a nasty stack trace when they happen.

A gemspec can be laoded, but that does not necessarily mean the has
been required. It's better to check for presence using expected
constants from the target library.
@mwear mwear changed the title Fix adapter examples Fix adapter examples and simplify setup Jan 25, 2020
@mwear mwear force-pushed the fix_adapter_examples branch from 95996da to 1fc760d Compare January 25, 2020 21:09
@mwear mwear merged commit fbc110d into open-telemetry:master Jan 27, 2020
fbogsany added a commit that referenced this pull request Mar 5, 2020
* rack: Add Gemfile, gemspec, version

* rack-test: "simple testing API for Rack apps"
* fix: "Could not find rake-12.3.3 in any of the sources"

* Add TracerMiddleware

* use dup._call env pattern
* RE: thread safety: "... easiest and most efficient way to do this is to
  dup the middleware object that is created in runtime."
* do we need 'quantization' now?

* Add Adapters::Rack::Adapter

* retain middleware names (feature exists in dd-trace-rb)

* tests: Add test_helper

* Add example: trace_demonstration

e.g.,
$ docker-compose run --rm ex-adapter-rack
bash-5.0$ ruby trace_demonstration.rb

* Add Rakefile

e.g.,
$ bundle exec rake test

* Add QueueTime

* from dd-trace-rb
* translate spec to minitest

* Add Adapters::Rack

* Update to 2020 copyright

* Initialize 'now' later

* Adapt to Instrumentation Auto Installer interface

* PR #164

* Fix example to use updated Instrumentation Auto Installer

* verified by running via, e.g.,
  $ docker-compose run --rm ex-adapter-rack
  bash-5.0$ bundle
  bash-5.0$ ruby trace_demonstration.rb

Expected: console output of span data

* Handle errors by setting span.status, leave a TODO and rescue clause

* Allow config[:quantization]

* to allow for lower cardinality span names

* Remove optional parent context extraction

* defeats purpose of opentelemetry, which *is* distributed tracing

* Resolve 'http.base_url' TODO

* add 'http.target'

* Resolve 'resource name' TODO

* it seems that dd-trace-rb's 'span.resource' is the same as 'span.name',
  at least in this case

* Resolve 'http.route' TODO

* in sinatra, data is available via env['sinatra.route'].split.last,
  but otherwise, doesn't seem to be readily available

* Note: missing 'span.set_error()' for now

* Resolve FrontendSpan TODOs

* resolve 'http_server.queue' TODO
* span kind of 'proxy' is not defined, yet

* Optimize allowed_request_headers

* reduce string allocations
* TIL: a nested 'before' block doesn't run before *each* test,
  but the top/first 'before' block does. (weird)

* Optimize allowed_response_headers()

* prevent unneeded string and object allocation
* once a header key is found, add it to the list of response headers
  to search for

* Optimize return of EMPTY_HASH frozen constant

* Refactor to avoid using dup._call(env)

* avoid using instance variables

* Add Appraisals, integrate into circleci

* Integrate rubocop, fix violations, add adapters to top-level rake task

* per work in #179

* Update example to use new config

* per #171, #177

* Rewrite examples

* one that is simple, no config options
  * demonstrate integration via 'Rack::Builder#use'
  * this is how ddtrace is currently working

* demonstrate integration using config[:application]

* ultimate goal: 0 config (automagic integration)

* Automatically patch Rack::Builder

* in tests, keep an unpatched ('untainted') version of Rack::Builder,
  restore before and after each test
* fix: ruby2.4: private method `define_method' called

* Port ddtrace Quantization::HTTP

* Integrate Util::Quantization

* Revert "Automatically patch Rack::Builder"

This reverts commit de91025.

# Conflicts:
#	adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb

* Add missing files needed for Bundler.require

* Update Rakefile

* Avoid patching config[:application] during installation

* config[:application] is patched in retain_middleware_names, in which
  case it is required if config[:retain_middleware_names] is truthy
* goal: leave .use call to the user

* Refine/optimize allowed_request_headers

* Refine/optimize allowed_response_headers

* Avoid circular require

* Use SDK.configure to streamline test_helper.rb setup

* Revert "Integrate Util::Quantization"

This reverts commit 99f44e8.

Discussed in SIG:
* avoid eager-quantization (heavy, potentially unwanted)
* defer to user preferences (via config)
* probably better to err on the side of 'too-specific' vs. 'too-general',
  since 'too-specific' can be made more general, but maybe not vice-versa

# Conflicts:
#	adapters/rack/lib/opentelemetry/adapters/rack/adapter.rb

* Revert "Port ddtrace Quantization::HTTP"

This reverts commit e9c021b.

* cleanup remnants that aren't used for now

* Fix example/trace_demonstration2.rb to integrate explicitly, with 'use'

* Update example/trace_demonstration2.rb documentation

* Optimize allowed_response_headers to avoid using Hash#detect

* Simplify allowed_{rack,request}_header_names to inline config

* Optimize to return EMPTY_HASH if allowed_{response,rack_request}_headers.empty?

* Adjust to context prop changes

* Remove unused variables

* Use kind: :server for both frontend and request span

* Make request_span parented by frontend_span

* explicitly manage frontend_span parent context, and prevent
  automatic span activation
* manage frontend_span life-cycle explicitly via a new context, using
  it as the request_span's parent, if it's available

* Implement using helpers to that in_span doesn't have to record and re-raise error

* Cleanup some URL wrapper methods

* goal: eliminate need for Rack::Request allocation

* Optimize: return without assigning local variable

* Just use http.{scheme,host,target} (remove url, base_url)

* Inline Rack::Request#fullpath

* Fix .circleci/config.yml after conflict

* Adjust error handling according to #184

* Rewrite to utilize in_span

* note that order of finished spans is now swapped

* Reduce comments that were more useful in development/review

* Update http.host to use HTTP_HOST or 'unknown'

Co-Authored-By: Matthew Wear <[email protected]>

* Update request_start_time to be number, not timestamp

Co-Authored-By: Matthew Wear <[email protected]>

* Remove request_span comment

* Remove 'service' attribute when creating frontend span

* value is potentially nil, which is not a valid attribute value
* also 'service' is not an official semantic convention and will
  probably come from the application resource in the future

* Change frontend_span to 'http_server.proxy', make request_span :internal

* request_span.kind is :internal if frontend_span is present
* future: change request_span :kind to ':proxy' if/when it gets added to spec

Co-authored-by: Matthew Wear <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants