[NO-TICKET] Fix "no signals" workaround detection when mariadb is in use #3362
Conversation
**What does this PR do?** This PR fixes #3334 by tweaking the libmysqlclient detection code to not trigger when the mariadb version of libmysqlversion is in use. The detection was triggering because the versioning schema used by mariadb is completely different from the libmysqlclient (e.g. latest releases report version 3.x, whereas mysql is 5.x or above). **Motivation:** For context, how we got here, is that legacy versions of the libmysqlclient library require the profiler "no signals" workaround to work without issues. To provide a seamless experience, we autodetect such versions and automatically apply the "no signals" workaround. Unfortunately, this detection code incorrectly flagged the libmysqlclient version provided by mariadb as being incompatible. In fact, that library has diverged a lot from the mysql version and we don't have any reports of issues that require the "no signals" workaround. **Additional Notes:** It's kinda fiddly to detect the mariadb version of libmysqlclient, see the added comments for more details. **How to test the change?** The change includes test coverage. To test it locally, I've additionally used the `dokken/centos-stream-9:latest` docker image, and then installed ruby + ruby-devel + gcc + make + MariaDB-devel from <https://mariadb.org/download/?t=repo-config&d=CentOS+Stream&v=11.2>. Fixes #3334
| header_version = Gem::Version.new(info[:header_version]) if info[:header_version] | ||
|
|
||
| !!(header_version && | ||
| libmysqlclient_version < Gem::Version.new('5.0.0') && |
There was a problem hiding this comment.
Based on https://github.com/brianmario/mysql2/blob/79f78f940685396f1b5f30ec502544bb7e3ba9cf/ext/mysql2/client.c#L148 it seems like one may be able to use mysql2 without linking/using the connector-c library in which case the reported version may be >= 10.0.0 even for MariaDB.
Doesn't seem like they have any better way to distinguish between mariadb and mysql themselves in the gem based on the version gymnastics being done around that line.
So looks like eventually any logic we add here will get out of date.
How crazy would it be to add a settings knob to complement this auto-detection and enabling users to force-set one flavour or the other?
There was a problem hiding this comment.
Ah, good call out!
Although looking at the code:
if (version >= 100000 // MariaDB (all versions numbered 10.x)
|| (version >= 30000 && version < 40000) // MariaDB Connector/C (all versions numbered 3.x)...I think we'd be safe, although somewhat by accident.
The problem in #3334 is that the version check is testing for libmysqlclient older than 8.x and using that to choose to enable the "no signals" workaround; because mariadb connector/c reports 3.x, the logic would trigger.
But if for mariadb without connector/c the version reported is 10.x, we're fine there too, because it's >= 8.x.
How crazy would it be to add a settings knob to complement this auto-detection and enabling users to force-set one flavour or the other?
There's already a settings.advanced.no_signals_workaround_enabled that can be set to true or false to override the autodetection.
Were you thinking of something like that or did you mean having something more specific to only override the mysql2 detection?
There was a problem hiding this comment.
Ah right. The workaround applies globally so no point making anything specific to mysql right? Just need to be aware of it when dealing with eventual support cases 👍
There was a problem hiding this comment.
Yeah the global one has worked so far. It's also documented in https://docs.datadoghq.com/profiler/profiler_troubleshooting/ruby/#unexpected-failures-or-errors-from-ruby-gems-that-use-native-extensions-in-dd-trace-rb-1110 .
What does this PR do?
This PR fixes #3334 by tweaking the libmysqlclient detection code to not trigger when the mariadb version of libmysqlversion is in use.
The detection was triggering because the versioning schema used by mariadb is completely different from the libmysqlclient (e.g. latest releases report version 3.x, whereas mysql is 5.x or above).
Motivation:
For context, how we got here, is that legacy versions of the libmysqlclient library require the profiler "no signals" workaround to work without issues. To provide a seamless experience, we autodetect such versions and automatically apply the "no signals" workaround.
Unfortunately, this detection code incorrectly flagged the libmysqlclient version provided by mariadb as being incompatible.
In fact, that library has diverged a lot from the mysql version and we don't have any reports of issues that require the "no signals" workaround.
Additional Notes:
It's kinda fiddly to detect the mariadb version of libmysqlclient, see the added comments for more details.
How to test the change?
The change includes test coverage. To test it locally, I've additionally used the
dokken/centos-stream-9:latestdocker image, and then installed ruby + ruby-devel + gcc + make + MariaDB-devel from https://mariadb.org/download/?t=repo-config&d=CentOS+Stream&v=11.2.For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Fixes #3334