Adds MySql.Data bytecode instrumentation #2686

zacharycmontoya · 2023-06-21T15:41:50Z

Why

Starting with version 8.0.33 of the MySql.Data nuget package, source instrumentation does not work because of how Activity.Current (and AsyncLocals in general) are propagated through the async method.

Fixes #2542

What

This PR adds automatic instrumentation all versions of MySql, including 8.0.33+. Changes include:

v8.0.33+: Automatic instrumentation of MySql.Data.MySqlClient.MySqlCommand.ExecuteReaderAsync
Lower versions: Automatic instrumentation of MySql.Data.MySqlClient.MySqlCommand.ExecuteReader
Automatic instrumentation of OpenTelemetry.Instrumentation.MySqlData.MySqlDataInstrumentation.ctor so we can consume options if the user configures instrumentation themselves

The instrumentation generates an Activity and decorates it in the same way that the OpenTelemetry.Instrumentation.MySqlData does.

It's important to note that this does not generate duplicate Activity objects. Only the Activity generated from the automatic instrumentation is finished and exported. Here's the order in which Activity events are generated:

When MySqlCommand.ExecuteReader/MySqlCommand.ExecuteReaderAsync begins, the automatic instrumentation starts a new Activity
- Note: Activity.Previous is either null or was created by another ActivitySource
Further within the callstack, OpenTelemetry.Instrumentation.MySqlData starts a new Activity
- Note: Activity.Previous is the automatic instrumentation Activity
When ExecuteReader/ExecuteReaderAsync exits, the automatic instrumentation stops the Activity it generates.
- Note: This changes Activity.Current to the value of Activity.Previous
When MySqlCommand.Close is called, OpenTelemetry.Instrumentation.MySqlData checks Activity.Current and if it was generated by its ActivitySource, it closes the Activity. With our automatic instrumentation, Activity.Current is either null or its ActivitySource is different, so the original Activity generated by the instrumentation library never closes.

Tests

The MySqlDataTests has been updated to use snapshot verification to make sure the data on the exported Activities is consistent from library version to library version. This has been used to validate that the Activity generated with the new bytecode instrumentation is identical to the Activity generated by previous instrumentation

Checklist

CHANGELOG.md is updated.
Documentation is updated.
New features are covered by tests.

Kielek · 2023-06-22T05:56:30Z

@zacharycmontoya, what's about the second option discussed one week ago? Maybe we could fully drop MySqlData instrumentation library and fully apply bytecode instrumentation?

…package where source instrumentation does not work

…n 8.0.32.1) to ensure the resulting span data is consistent

…OpenTelemetry.Instrumentation.MySqlData.MySqlDataInstrumentationOptions. Next, we can try to set EnableConnectionLevelAttributes=true and see if the implementation works correctly

…ySqlDataInstrumentation constructor so we can access the MySqlDataInstrumentationOptions at the end of the method call, to correctly decorate our MySqlData span. In this commit, also edit the options in MySqlDataInitializer and the resulting snapshot, so we can verify that new instrumentation works correctly. This will be reverted in a later commit

… and scrub the mysql port so we get consistent results

…nitializer and update the snapshot

…st version

…every version going back to 6.10.7 passes the snapshot test. Untested versions are versions without .NET Core support, which include: - 6.7.9 - 6.8.8 - 6.9.12

src/OpenTelemetry.AutoInstrumentation/Instrumentations/MySqlData/MySqlDataCommon.cs

…AutoInstrumentation.MySqlData

…tation package, relying entirely on the new bytecode instrumentation

Kielek

First round of review. I need to revisit how the instrumentation is implemented.

docs\config.md needs to be update - all versions are covered by bytecode instrumentation
CHANGELOG.md needs a record that we have changed the way how mysql is instrumented + info about fixes known issue from rc.1

Kielek · 2023-08-10T05:29:27Z

src/CommonExcludedAssets.props

-    <!-- MySql.Data is required by OpenTelemetry.Instrumentation.MySqlData. -->
-    <PackageReference Include="MySql.Data" ExcludeAssets="all" />
-    <!-- System.Configuration.ConfigurationManager is tranistive dependency required by OpenTelemetry.Instrumentation.MySqlData. -->
-    <PackageReference Include="System.Configuration.ConfigurationManager" ExcludeAssets="all" />
-    <!-- System.Security.Cryptography.ProtectedData is tranistive dependency required by OpenTelemetry.Instrumentation.MySqlData. -->
-    <PackageReference Include="System.Security.Cryptography.ProtectedData" ExcludeAssets="all" />
-    <!-- System.Drawing.Common is tranistive dependency required by OpenTelemetry.Instrumentation.MySqlData. -->
-    <PackageReference Include="System.Drawing.Common" ExcludeAssets="all" />
-    <!-- System.Windows.Extensions is tranistive dependency required by OpenTelemetry.Instrumentation.MySqlData. -->
-    <PackageReference Include="System.Windows.Extensions" ExcludeAssets="all" />
-    <!-- Microsoft.Win32.SystemEvents is tranistive dependency required by OpenTelemetry.Instrumentation.MySqlData. -->
-    <PackageReference Include="Microsoft.Win32.SystemEvents" ExcludeAssets="all" />


All this versions should be removed also from src/Directory.Packages.props CommonExcludedAssets.props section

Kielek · 2023-08-10T05:31:04Z

test/IntegrationTests/LibraryVersions.g.cs

@@ -114,7 +114,7 @@ public static class LibraryVersion
        new object[] { string.Empty }
 #else
        new object[] { "6.10.7" },
-        new object[] { "8.0.32.1" },
+        new object[] { "8.1.0" },


Add 8.0.33 version to tools\LibraryVersionsGenerator\PackageVersionDefinitions.cs. We should keep minimal problematic version in our tests.

Then regenerate files.

Kielek · 2023-08-10T05:32:37Z

test/test-applications/integrations/TestApplication.MySqlData/TestApplication.MySqlData.csproj


  <PropertyGroup>
    <OutputType>Exe</OutputType>
  </PropertyGroup>

  <ItemGroup>
+    <PackageReference Include="Google.Protobuf" VersionOverride="3.21.9" />


Why you need to override?
test\Directory.Packages.props defines it as 3.24.0.

Kielek · 2023-08-10T05:42:07Z

test/IntegrationTests/BuildTests.DistributionStructure_windows.verified.txt

DistributionStructure tests are failing. Mostly due to changes in Excluded Assets.
We need to determine which packages brings following dependencies

\net\Microsoft.Win32.SystemEvents.dll, \net\System.Configuration.ConfigurationManager.dll, \net\System.Drawing.Common.dll, \net\System.Security.Cryptography.ProtectedData.dll, \net\System.Windows.Extensions.dll, \net\runtimes\unix\lib\netcoreapp3.0\System.Drawing.Common.dll, \net\runtimes\win\lib\netcoreapp3.0\Microsoft.Win32.SystemEvents.dll, \net\runtimes\win\lib\netcoreapp3.0\System.Drawing.Common.dll, \net\runtimes\win\lib\netcoreapp3.0\System.Windows.Extensions.dll, \net\runtimes\win\lib\netstandard2.0\System.Security.Cryptography.ProtectedData.dll,

Kielek · 2023-08-10T06:04:24Z

src/OpenTelemetry.AutoInstrumentation/Instrumentation.cs

                    break;
-                case TracerInstrumentation.MySqlData:
-                    DelayedInitialization.Traces.AddMySqlClient(LazyInstrumentationLoader, pluginManager);
-                    break;
                case TracerInstrumentation.EntityFrameworkCore:


Suggested change

break;

case TracerInstrumentation.MySqlData:

DelayedInitialization.Traces.AddMySqlClient(LazyInstrumentationLoader, pluginManager);

break;

case TracerInstrumentation.EntityFrameworkCore:

break;

case TracerInstrumentation.MySqlData:

break;

case TracerInstrumentation.EntityFrameworkCore:

Otherwise it will leads to

Logger.Warning($"Configured trace instrumentation type is not supported: {instrumentation}"); if (FailFastSettings.Value.FailFast) { throw new NotSupportedException($"Configured trace instrumentation type is not supported: {instrumentation}"); }

Kielek · 2023-08-10T06:22:02Z

src/OpenTelemetry.AutoInstrumentation/Instrumentations/MySqlData/ExecuteReaderIntegration.cs

+    returnTypeName: "System.Threading.Tasks.Task`1<MySql.Data.MySqlClient.MySqlDataReader>",
+    parameterTypeNames: new[] { "System.Data.CommandBehavior" },
+    minimumVersion: "6.10.7",
+    maximumVersion: "8.0.32",


Just to double check, what's about 8.0.32.1?

Kielek · 2023-08-10T06:24:04Z

...erators/SourceGenerators.InstrumentationDefinitionsGenerator/InstrumentationDefinitions.g.cs

Whats about other instrumentations defined in upsteam?
For now it seems that here only ExecuteReader is instrumented: https://github.com/DataDog/dd-trace-dotnet/pull/4109/files#diff-b7a5ab305895159ec230fc6ecb7bec90b944fb30ddee8aa7558d141daa42376aR222-R255

nrcventura · 2023-08-10T19:31:01Z

@zacharycmontoya, what's about the second option discussed one week ago? Maybe we could fully drop MySqlData instrumentation library and fully apply bytecode instrumentation?

This might be necessary. The Activity class is disposable, and when disposing will stop the activity. The Dispose method also has a call to suppress finalization, which makes me think there are versions with a finalizer or there are plans to include a finalizer. And if that is the case, we could see both activities collected.

pjanotti · 2023-08-15T02:49:22Z

Closing this one given that #2836 was merged. Please, re-open if appropriate. /cc @zacharycmontoya @Kielek

zacharycmontoya added 9 commits August 4, 2023 11:32

Update CallTargetState to accept a nullable activity in the public API

602a4a2

Add initial MySql.Data instrumentation for the affected 8.0.33 nuget …

c071dde

…package where source instrumentation does not work

Implement snapshot testing for MySqlDataTests (testing against versio…

4b3fcf1

…n 8.0.32.1) to ensure the resulting span data is consistent

Implement peer.service with the initial infrastructure for obtaining …

ddb0c50

…OpenTelemetry.Instrumentation.MySqlData.MySqlDataInstrumentationOptions. Next, we can try to set EnableConnectionLevelAttributes=true and see if the implementation works correctly

Add a new Verify converter for OpenTelemetry.Proto.Common.V1.KeyValue…

5093b16

… and scrub the mysql port so we get consistent results

Revert the MySqlDataInstrumentationOptions modification in MySqlDataI…

2172d78

…nitializer and update the snapshot

Reset the csproj changes and update the tested MySql.Data to the late…

298be24

…st version

Add ExecuteReaderIntegration to cover older versions. At this point, …

f8ecacf

…every version going back to 6.10.7 passes the snapshot test. Untested versions are versions without .NET Core support, which include: - 6.7.9 - 6.8.8 - 6.9.12

zacharycmontoya force-pushed the zach/mysql-data branch from 985f7e6 to f8ecacf Compare August 9, 2023 02:19

Update LibraryVersions

cc88f43

zacharycmontoya changed the title ~~[WIP] Adds MySql.Data bytcode instrumentation~~ [WIP] Adds MySql.Data bytecode instrumentation Aug 9, 2023

zacharycmontoya marked this pull request as ready for review August 9, 2023 15:58

zacharycmontoya requested a review from a team as a code owner August 9, 2023 15:58

Only compile MySqlData types for net6.0 or higher

df3341f

zacharycmontoya commented Aug 9, 2023

View reviewed changes

src/OpenTelemetry.AutoInstrumentation/Instrumentations/MySqlData/MySqlDataCommon.cs Outdated Show resolved Hide resolved

zacharycmontoya changed the title ~~[WIP] Adds MySql.Data bytecode instrumentation~~ Adds MySql.Data bytecode instrumentation Aug 9, 2023

zacharycmontoya added 2 commits August 9, 2023 10:08

Update bytecode instrumentation ActivitySource name to OpenTelemetry.…

d4bcef1

…AutoInstrumentation.MySqlData

Remove OpenTelemetry.Instrumentation.MySqlData from the autoinstrumen…

2a0e73f

…tation package, relying entirely on the new bytecode instrumentation

github-actions bot requested a review from theletterf August 9, 2023 17:14

Kielek reviewed Aug 10, 2023

View reviewed changes

Kielek mentioned this pull request Aug 14, 2023

MySql.Data - natively supported by library #2836

Merged

3 tasks

pjanotti closed this Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds MySql.Data bytecode instrumentation #2686

Adds MySql.Data bytecode instrumentation #2686

zacharycmontoya commented Jun 21, 2023 •

edited

Kielek commented Jun 22, 2023

Kielek left a comment

Kielek Aug 10, 2023

Kielek Aug 10, 2023

Kielek Aug 10, 2023

Kielek Aug 10, 2023

Kielek Aug 10, 2023

Kielek Aug 10, 2023

Kielek Aug 10, 2023

nrcventura commented Aug 10, 2023

pjanotti commented Aug 15, 2023

Adds MySql.Data bytecode instrumentation #2686

Adds MySql.Data bytecode instrumentation #2686

Conversation

zacharycmontoya commented Jun 21, 2023 • edited

Why

What

Tests

Checklist

Kielek commented Jun 22, 2023

Kielek left a comment

Choose a reason for hiding this comment

Kielek Aug 10, 2023

Choose a reason for hiding this comment

Kielek Aug 10, 2023

Choose a reason for hiding this comment

Kielek Aug 10, 2023

Choose a reason for hiding this comment

Kielek Aug 10, 2023

Choose a reason for hiding this comment

Kielek Aug 10, 2023

Choose a reason for hiding this comment

Kielek Aug 10, 2023

Choose a reason for hiding this comment

Kielek Aug 10, 2023

Choose a reason for hiding this comment

nrcventura commented Aug 10, 2023

pjanotti commented Aug 15, 2023

zacharycmontoya commented Jun 21, 2023 •

edited