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

Improve controller instrumentation and deprecate option exception_controller #2726

Merged
merged 13 commits into from
Mar 31, 2023

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Mar 28, 2023

Why?

This PR addresses issues (#2709 , #2611 , #2567) we've had for instrumenting an exception handling controller.

  1. The trace resource being overwritten by the exception handling controller, make it hard understand the origin of error.
  2. Configuration for exception_controller with constant does not work properly with zeitwerk autoloading.

What?

After careful examination of the problem, I have concluded that the current behaviour of option exception_controller is bringing more harm than benefit, because of the fundamental problem to answer the question about what is the strategy to decide whether to overwrite trace resource?

🚫 It should not be made by based on whether the current controller is handling the request being defined as an exceptional controller, since a request could be sent to the path directly (such case: overwrite trace resource).
The decision should be made by whether an exception being redirected by ActionDispatch::ShowException.

The changes are made to depend on the default behaviour for ActionDispatch::ShowException and make the option exception_controller obsolete. We decided to deprecate the option and no longer required to solve the problem with zeitwerk autoloading.

We recommend our users to remove exception_controller option from the configuration.


Snapshot of changes

Screenshot 2023-03-28 at 11 42 21

@TonyCTHsu TonyCTHsu changed the title Improve exception controller instrumentation Improve trace resource assignment for controller instrumentation and deprecate option exception_controller Mar 29, 2023
@TonyCTHsu TonyCTHsu changed the title Improve trace resource assignment for controller instrumentation and deprecate option exception_controller Improve controller instrumentation and deprecate option exception_controller Mar 29, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2726 (145d81f) into master (3c4c4ba) will increase coverage by 0.01%.
The diff coverage is 98.59%.

@@            Coverage Diff             @@
##           master    #2726      +/-   ##
==========================================
+ Coverage   98.06%   98.07%   +0.01%     
==========================================
  Files        1210     1215       +5     
  Lines       66456    67025     +569     
  Branches     2972     2994      +22     
==========================================
+ Hits        65172    65738     +566     
- Misses       1284     1287       +3     
Impacted Files Coverage Δ
spec/datadog/core/telemetry/collector_spec.rb 100.00% <ø> (ø)
spec/datadog/core/telemetry/v1/app_event_spec.rb 100.00% <ø> (ø)
...atadog/core/telemetry/v1/telemetry_request_spec.rb 100.00% <ø> (ø)
lib/datadog/core/remote/component.rb 88.00% <66.66%> (-7.46%) ⬇️
...og/tracing/contrib/rails/configuration/settings.rb 97.43% <80.00%> (+0.29%) ⬆️
lib/datadog/core/utils/hash.rb 84.61% <84.61%> (ø)
...cing/contrib/action_pack/configuration/settings.rb 96.00% <85.71%> (-4.00%) ⬇️
.../datadog/appsec/contrib/rack/request_middleware.rb 95.29% <90.90%> (-0.81%) ⬇️
lib/datadog/core/remote/client.rb 98.16% <93.10%> (-1.84%) ⬇️
lib/datadog/appsec/processor.rb 95.93% <95.65%> (+0.65%) ⬆️
... and 28 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TonyCTHsu TonyCTHsu added this to the 1.11.0 milestone Mar 31, 2023
@TonyCTHsu TonyCTHsu merged commit 106e7ec into master Mar 31, 2023
@TonyCTHsu TonyCTHsu deleted the tonycthsu/error_controller branch March 31, 2023 09:15
@lloeki lloeki modified the milestones: 1.11.0, 1.11.0.beta1 Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
4 participants