Add rubocop to adapters #179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mwear
requested review from
bai,
dazuma,
elskwid,
fbogsany and
luvtechno
as code owners
bai
approved these changes
Jan 28, 2020
fbogsany
approved these changes
Jan 28, 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The instrumentation adapters did not have rubocop setup and as a result had a number of violations. This PR sets up rubocop and fixes the violations for the Faraday and Sinatra instrumentation adapters. It also adds them to the top-level Rakefile so that tests are run automatically.