[PROF-3514] Skip CPU time instrumentation if logging gem is detected #1557
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
Instead, customers will get the following log message: ``` INFO -- ddtrace: [ddtrace] (/app/lib/ddtrace/profiling/tasks/setup.rb :44:in `activate_cpu_extensions') CPU time profiling skipped because native CPU time is not supported: The `logging` gem is installed and its thread inherit context feature is enabled. Please add LOGGING_INHERIT_CONTEXT=false to your application's environment variables to disable the conflicting `logging` gem feature. See TwP/logging#230 for details. Profiles containing Wall time will still be reported. ``` This is a repeat of #1362 -- the logging gem also monkey patches `Thread` initialization using `alias_method`, which is incompatible with our instrumentation that uses `preload`. For details, see TwP/logging#230 where we submitted a fix to the upstream gem. Right now the only way to avoid this issue and still enable proflier is to either remove the logging gem or to disable the feature using the `LOGGING_INHERIT_CONTEXT` environment variable. If/when the gem creators accept our proposed change and release it, we'll need to revise this to take that into consideration, as we do for rollbar. NOTE: I'm not entirely happy with the complexity that is accumulating for the tests, but I've decided to not refactor it yet. If we need to add more gems, we should definitely refactor the tests. My hope is that soon we can get rid of our `Thread` monkey patches, and thus get rid of all of this instead.
ivoanjo
force-pushed
the
fix/skip-cpu-time-instrumentation-if-logging-is-around
branch
from
add4289 to
cef8067
Compare
Codecov Report
@@ Coverage Diff @@
## master #1557 +/- ##
=======================================
Coverage 98.24% 98.24%
=======================================
Files 878 878
Lines 42429 42452 +23
=======================================
+ Hits 41683 41706 +23
Misses 746 746
Continue to review full report at Codecov.
|
marcotc
approved these changes
Jun 17, 2021
ivoanjo
deleted the
fix/skip-cpu-time-instrumentation-if-logging-is-around
branch
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.
...instead, customers will get a log message as follows:
This is a repeat of #1362 -- the logging gem also monkey patches
Threadinitialization usingalias_method, which is incompatible with our instrumentation that usespreload.For details, see TwP/logging#230 where we submitted a fix to the upstream gem.
Right now the only way to avoid this issue and still enable proflier is to either remove the logging gem or to disable the feature using the
LOGGING_INHERIT_CONTEXTenvironment variable.If/when the gem creators accept our proposed change and release it, we'll need to revise this to take that into consideration, as we do for rollbar.
NOTE: I'm not entirely happy with the complexity that is accumulating for the tests, but I've decided to not refactor it yet. If we need to add more gems, we should definitely refactor the tests. My hope is that soon we can get rid of our
Threadmonkey patches, and thus get rid of all of this instead.