[PROF-6447] Improve detection of mysql2 gem incompatibilities with profiler #2770
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
…ofiler **What does this PR do?**: As part of #2702 (more specifically, in a7d3b73) we added a check for the `mysql2` gem which avoided turning on the new profiler when we detected it. This check was a bit heavy-handed because the known incompatibility between the `mysql2` gem and the profiler only happens if the `mysql2` gem is using a legacy version of the `libmysqlclient` C library. (Specifically, a version prior to 8.0.0) In this PR, we improve the check to _actually_ check the `libmysqlclient` version. Thus, we will only fall back to using the legacy profiler as needed, not always as before. **Motivation**: The `mysql2` gem is a commonly-used database driver for Ruby (and Ruby on Rails) applications, and we don't want these customers to get stuck using the old profiler. **Additional Notes**: As part of the check, we need to `require 'mysql2'` during profiler initialization (since we need to call a method provided by the gem). In the rare just-in-case situation that this causes issues and customers don't want this to happen, this PR also introduces a new option to disable this behavior: * `DD_PROFILING_SKIP_MYSQL2_CHECK` via environment variable * `c.profiling.advanced.skip_mysql2_check` via code **How to test the change?**: Test coverage is included. To test this with the actual `mysql2` gem, our CI images have a modern version of `libmysqlclient` (specifically, the mariadb variant): ``` $ DD_PROFILING_ENABLED=true DD_TRACE_DEBUG=true bundle exec ddtracerb exec ruby -e "sleep 1" DEBUG -- ddtrace: [ddtrace] (/app/lib/datadog/profiling/component.rb:205:in `compatible_libmysqlclient_version?') Requiring `mysql2` to check if the `libmysqlclient` version it uses is compatible with profiling DEBUG -- ddtrace: [ddtrace] (/app/lib/datadog/profiling/component.rb:218:in `compatible_libmysqlclient_version?') The `mysql2` gem is using a compatible version of the `libmysqlclient` library (10.5.15) DEBUG -- ddtrace: [ddtrace] (/app/lib/datadog/core/configuration/components.rb:92:in `startup!') Profiling started DEBUG -- ddtrace: [ddtrace] (/app/lib/datadog/profiling/collectors/cpu_and_wall_time_worker.rb:58:in `block in start') Starting thread for: #<Datadog::Profiling::Collectors::CpuAndWallTimeWorker:0x00005e979dd5e498> ``` ...and you can use an older Ubuntu 18.04 image to see the failing case: ``` $ DD_PROFILING_ENABLED=true DD_TRACE_DEBUG=true bundle exec ddtracerb exec ruby -e "sleep 1" DEBUG -- ddtrace: [ddtrace] (/working/lib/datadog/profiling/component.rb:205:in `compatible_libmysqlclient_version?') Requiring `mysql2` to check if the `libmysqlclient` version it uses is compatible with profiling DEBUG -- ddtrace: [ddtrace] (/working/lib/datadog/profiling/component.rb:218:in `compatible_libmysqlclient_version?') The `mysql2` gem is using an incompatible version of the `libmysqlclient` library (5.7.41) WARN -- ddtrace: [ddtrace] (/working/lib/datadog/profiling/component.rb:170:in `enable_new_profiler?') Falling back to legacy profiler because an incompatible version of the mysql2 gem is installed. Older versions of libmysqlclient (the C library used by the mysql2 gem) have a bug in their signal handling code that the new profiler can trigger. This bug (https://bugs.mysql.com/bug.php?id=83109) is fixed in libmysqlclient versions 8.0.0 and above. DEBUG -- ddtrace: [ddtrace] (/working/lib/datadog/core/configuration/components.rb:92:in `startup!') Profiling started DEBUG -- ddtrace: [ddtrace] (/working/lib/datadog/core/workers/async.rb:130:in `start_worker') Starting thread for: #<Datadog::Profiling::Collectors::OldStack:0x00005604f7ac46b0> ```
These apps previously used the `mysql2` gem and thus were forced to use the old profiler; with the improved detection of incompatible versions of the `mysql2` gem these apps can now use the new profiler!
github-actions
bot
added
core
GustavoCaso
reviewed
Apr 12, 2023
lib/datadog/profiling/component.rb
Outdated
| @@ -166,14 +166,12 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:) | |||
|
|
|||
| return false if RUBY_VERSION.start_with?('2.3.', '2.4.', '2.5.') | |||
|
|
|||
| if Gem.loaded_specs['mysql2'] | |||
| if Gem.loaded_specs['mysql2'] && !compatible_libmysqlclient_version?(settings) | |||
There was a problem hiding this comment.
Should we encapsulate the Gem.loaded_specs['mysql2'] inside the compatible_libmysqlclient_version?(settings) method?
There was a problem hiding this comment.
I'm not entirely convinced by this, because it's kinda weird to think about "what does compatible_libmysqlclient_version? mean when mysql2 is not installed"?
Marking it as incompatible would trigger the warning. Marking "mysql2 is not installed" as compatible would work, but seems a bit stretching the definition of compatible.
It seems to me that the condition here should always have two components -- is it available, and if yes, is it compatible; I could perhaps extract an is_mysql2_available? but since it's a trivial check, I'm not sure it's worth it.
GustavoCaso
reviewed
Apr 12, 2023
lib/datadog/profiling/component.rb
Outdated
| # | ||
| # The `mysql2` gem's `info` method can be used to determine which `libmysqlclient` version is in use, and thus to | ||
| # detect if it's safe for the profiler to use signals or if we need to employ a fallback. | ||
| private_class_method def self.compatible_libmysqlclient_version?(settings) |
There was a problem hiding this comment.
We could rename the method to incompatible_libmysqlclient_version? and the return value 😄 that way in the check above we do not have to negate the result
GustavoCaso
reviewed
Apr 12, 2023
| @@ -0,0 +1,5 @@ | |||
| module Mysql2 | |||
| class Client | |||
| def self.info: () -> { version: ::String } | |||
There was a problem hiding this comment.
Took me sometime to find where that method is defined 🤣
https://github.com/brianmario/mysql2/blob/master/ext/mysql2/client.c#L1543
There was a problem hiding this comment.
Yeah, this is why I usually prefix native methods with _native_ -- to hint were they are coming from and avoid a few minutes of "but wait... where is this method?".
GustavoCaso
approved these changes
Apr 12, 2023
This spec was assuming that neither `mysql2` nor `rugged` were in the `Gemfile`, which is the case usually, but I had added them for local testing and run into issues. This change makes the test resilient to such environments (and hopefully less confusing/error-prone).
ivoanjo
added a commit
that referenced
this pull request
Jul 10, 2023
**What does this PR do?**: This PR tweaks the libmysqlclient version detection code added in #2770 to also work when the `mysql2-aurora` gem is in use. **Motivation**: The way that the mysql2-aurora gem installs itself leads to the "no signals" workaround (#2873) being incorrectly enabled for customers that do have a modern version of libmysqlclient. **Additional Notes**: The mysql2-aurora gem likes to monkey patch itself in replacement of `Mysql2::Client`, and uses `method_missing` to delegate to the original BUT unfortunately does not implement `respond_to_missing?` and thus one of our checks (`respond_to?(:info)`) was incorrectly failing. **How to test the change?**: This change includes code coverage. This can also be reproduced easily by adding the `mysql2-aurora` gem to the `Gemfile` and then running a trivial Ruby app: ``` $ DD_PROFILING_ENABLED=true DD_TRACE_DEBUG=true bundle exec ruby -e "require 'mysql2/aurora'; require 'datadog/profiling/preload'" # Before DEBUG -- ddtrace: [ddtrace] Requiring `mysql2` to check if the `libmysqlclient` version it uses is compatible with profiling WARN -- ddtrace: [ddtrace] Enabling the profiling "no signals" workaround because an incompatible version of the mysql2 gem is installed. Profiling data will have lower quality. To fix this, upgrade the libmysqlclient in your OS image to version 8.0.0 or above. # After DEBUG -- ddtrace: [ddtrace] Requiring `mysql2` to check if the `libmysqlclient` version it uses is compatible with profiling DEBUG -- ddtrace: [ddtrace] The `mysql2` gem is using a compatible version of the `libmysqlclient` library (8.0.33) ```
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.
What does this PR do?:
As part of #2702 (more specifically, in a7d3b73) we added a check for the
mysql2gem which avoided turning on the new profiler when we detected it.This check was a bit heavy-handed because the known incompatibility between the
mysql2gem and the profiler only happens if themysql2gem is using a legacy version of thelibmysqlclientC library. (Specifically, a version prior to 8.0.0)In this PR, we improve the check to actually check the
libmysqlclientversion. Thus, we will only fall back to using the legacy profiler as needed, not always as before.Motivation:
The
mysql2gem is a commonly-used database driver for Ruby (and Ruby on Rails) applications, and we don't want these customers to get stuck using the old profiler.Additional Notes:
As part of the check, we need to
require 'mysql2'during profiler initialization (since we need to call a method provided by the gem).In the rare just-in-case situation that this causes issues and customers don't want this to happen, this PR also introduces a new option to disable this behavior:
DD_PROFILING_SKIP_MYSQL2_CHECKvia environment variablec.profiling.advanced.skip_mysql2_checkvia codeHow to test the change?:
Test coverage is included.
To test this with the actual
mysql2gem, our CI images have a modern version oflibmysqlclient(specifically, the mariadb variant):...and you can use an older Ubuntu 18.04 image to see the failing case: