Preliminary error recording #184
Merged
Conversation
This file contains hidden or 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
Right now the implementation is speculatively based around recording errors as events on spans. The implementation is likely to change once errors are officially spec'd, but this allows to start recording them now.
mwear
requested review from
bai,
dazuma,
elskwid,
fbogsany and
luvtechno
as code owners
elskwid
reviewed
Feb 19, 2020
| rescue Exception => e # rubocop:disable Lint/RescueException | ||
| span.record_error(e) | ||
| span.status = Status.new(Status::UNKNOWN_ERROR, | ||
| description: "Unhandled exception of type: #{e.class}") |
duonoid
reviewed
Feb 19, 2020
| # Record an error during the execution of this span. Multiple errors | ||
| # can be recorded on a span. | ||
| # | ||
| # @param [Exception] error The error to recorded |
Contributor
There was a problem hiding this comment.
doc nit: s/to recorded/to be recorded/
grantbdev
approved these changes
Feb 19, 2020
Member
Author
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
4 participants
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.
Based on #181 and our discussions at the SIG meeting, we decided to go ahead with preliminary error recording. This PR adds a
Span#record_errorthat records an error as an event with the semantic conventions laid out in open-telemetry/opentelemetry-specification#427. The stack trace is stored as a newline delimited string until we have support for array-value attributes. This also updatesTracer#in_spanto record and reraise exceptions.We expect that the implementation of
Span#record_errorwill change as it becomes officially spec'd, but this will give us something that we can use now. It will be helpful for a couple of open PRs: #166 and #176.