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.

Sign up for GitHub

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

Jump to bottom

Fix InvalidOperationException in DBM propagation #6233

Merged
merged 15 commits into from
Nov 5, 2024
Merged

Fix InvalidOperationException in DBM propagation #6233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
50923a8
Fix DBM propagation for Transaction and Connection
bouwkast Nov 4, 2024
625459f
Remove if branch
bouwkast Nov 4, 2024
07827d6
Add note about Npgsql exception
bouwkast Nov 4, 2024
53b55fe
Add sqlite snapshots and fix number of spans
bouwkast Nov 4, 2024
e9d2710
Update DBM mocks
bouwkast Nov 4, 2024
2d9834b
Fix SystemDataSqliteTests and add snapshots
bouwkast Nov 4, 2024
fb097f5
Fix OracleMDA
bouwkast Nov 4, 2024
7368b5f
Update mysql tests
bouwkast Nov 5, 2024
a0b53cd
Fix for MySql v9
bouwkast Nov 5, 2024
fa35ba2
Fix snapshots
bouwkast Nov 5, 2024
8ee054a
Don't use transaction scope for MySql
bouwkast Nov 5, 2024
0984b5d
Revert "Fix snapshots"
bouwkast Nov 5, 2024
e31e8f2
Revert "Fix for MySql v9"
bouwkast Nov 5, 2024
d4b4c44
Revert "Update mysql tests"
bouwkast Nov 5, 2024
9929194
Conditionally enabled/disable Transcation in tests
bouwkast Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions tracer/src/Datadog.Trace.Trimming/build/Datadog.Trace.Trimming.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,13 +204,15 @@
<type fullname="System.Data.CommandType" />
<type fullname="System.Data.Common.DbCommand" />
<type fullname="System.Data.Common.DbConnectionStringBuilder" />
<type fullname="System.Data.ConnectionState" />
<type fullname="System.Data.DbType" />
<type fullname="System.Data.IDataParameter" />
<type fullname="System.Data.IDataParameterCollection" />
<type fullname="System.Data.IDataRecord" />
<type fullname="System.Data.IDbCommand" />
<type fullname="System.Data.IDbConnection" />
<type fullname="System.Data.IDbDataParameter" />
<type fullname="System.Data.IDbTransaction" />
</assembly>
<assembly fullname="System.Data.SqlClient" />
<assembly fullname="System.Data.SQLite" />
Expand Down
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/DbScopeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ internal static class DbScopeFactory
}

// try context injection only after comment injection, so that if it fails, we still have service level propagation
var traceParentInjectedInContext = DatabaseMonitoringPropagator.PropagateDataViaContext(tracer.Settings.DbmPropagationMode, integrationId, command.Connection, scope.Span);
var traceParentInjectedInContext = DatabaseMonitoringPropagator.PropagateDataViaContext(tracer.Settings.DbmPropagationMode, integrationId, command, scope.Span);
if (traceParentInjectedInComment || traceParentInjectedInContext)
{
tags.DbmTraceInjected = "true";
Expand Down
22 changes: 19 additions & 3 deletions tracer/src/Datadog.Trace/DatabaseMonitoring/DatabaseMonitoringPropagator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,21 +113,37 @@ internal static string PropagateDataViaComment(DbmPropagationLevel propagationSt
/// Currently only working for MSSQL (uses an instruction that is specific to it)
/// </summary>
/// <returns>True if the traceparent information was set</returns>
internal static bool PropagateDataViaContext(DbmPropagationLevel propagationLevel, IntegrationId integrationId, IDbConnection? connection, Span span)
internal static bool PropagateDataViaContext(DbmPropagationLevel propagationLevel, IntegrationId integrationId, IDbCommand command, Span span)
{
if (propagationLevel != DbmPropagationLevel.Full || integrationId != IntegrationId.SqlClient || connection == null)
if (propagationLevel != DbmPropagationLevel.Full || integrationId != IntegrationId.SqlClient)
{
return false;
}

// NOTE: For Npgsql command.Connection throws NotSupportedException for NpgsqlDataSourceCommand (v7.0+)
// Since the feature isn't available for Npgsql we avoid this due to the integrationId check above
if (command.Connection == null)
{
return false;
}

if (command.Connection.State != ConnectionState.Open)
{
Log.Debug("PropagateDataViaContext did not have an Open connection, so it could not propagate Span data for DBM. Connection state was {ConnectionState}", command.Connection.State);

return false;
}

var stopwatch = System.Diagnostics.Stopwatch.StartNew();

const byte version = 0; // version can have a maximum value of 15 in the current format
var sampled = SamplingPriorityValues.IsKeep(span.Context.TraceContext.GetOrMakeSamplingDecision());
Copy link
Member

@lucaspimentel lucaspimentel Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of the changes in this PR, but are we ok forcing a sampling decision here? What is the sampling decision in the injected context used for? Does it affect sampling of other data aside from the trace (i.e. DBM data)? Is it propagated to other services?

We may want to use the sampling decision if there is one already, but otherwise not force it to happen yet, since we generally want to delay it until it's required for downstream propagation or when sending the trace to the agent. Does BuildContextValue() support sampled: null?

edit: not something to change in this PR, anyway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think the DBM context doesn't support "no sampling decision yet," so never mind, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the sampling decision in the injected context used for? Does it affect sampling of other data aside from the trace (i.e. DBM data)? Is it propagated to other services?

Honestly no idea, I can try to follow up with other DBM people

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it may be worth asking them, I don't think they do anything with the sampling decision so far, it's just written to do it like papa (the other propagation mechanisms)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

papa?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean take example on what's here before. It's a metaphor ^^

var contextValue = BuildContextValue(version, sampled, span.SpanId, span.TraceId128);

using (var injectionCommand = connection.CreateCommand())
using (var injectionCommand = command.Connection.CreateCommand())
{
// if there is a Transaction we need to copy it or our ExecuteNonQuery will throw
injectionCommand.Transaction = command.Transaction;
Comment on lines +145 to +146
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yikes, good catch

injectionCommand.CommandText = SetContextCommand;

var parameter = injectionCommand.CreateParameter();
Expand Down
21 changes: 20 additions & 1 deletion tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/AdoNet/MicrosoftDataSqliteTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using Datadog.Trace.Configuration;
using Datadog.Trace.TestHelpers;
using VerifyXunit;
using Xunit;
using Xunit.Abstractions;

namespace Datadog.Trace.ClrProfiler.IntegrationTests.AdoNet
{
[UsesVerify]
public class MicrosoftDataSqliteTests : TracingIntegrationTest
{
public MicrosoftDataSqliteTests(ITestOutputHelper output)
Expand Down Expand Up @@ -47,7 +50,7 @@ public async Task SubmitsTraces(string packageVersion, string metadataSchemaVers
return;
}
#endif
const int expectedSpanCount = 91;
const int expectedSpanCount = 105;
const string dbType = "sqlite";
const string expectedOperationName = dbType + ".query";

Expand All @@ -64,6 +67,22 @@ public async Task SubmitsTraces(string packageVersion, string metadataSchemaVers
ValidateIntegrationSpans(spans, metadataSchemaVersion, expectedServiceName: clientSpanServiceName, isExternalSpan);

telemetry.AssertIntegrationEnabled(IntegrationId.Sqlite);

var settings = VerifyHelper.GetSpanVerifierSettings();
settings.AddRegexScrubber(new Regex("Sqlite-Test-[a-zA-Z0-9]{32}"), "System-Data-SqlClient-Test-GUID");
settings.AddSimpleScrubber("out.host: localhost", "out.host: sqlserver");
settings.AddSimpleScrubber("out.host: (localdb)\\MSSQLLocalDB", "out.host: sqlserver");
settings.AddSimpleScrubber("out.host: sqledge_arm64", "out.host: sqlserver");
settings.AddSimpleScrubber("peer.service: localhost", "peer.service: sqlserver");
settings.AddSimpleScrubber("peer.service: (localdb)\\MSSQLLocalDB", "peer.service: sqlserver");
settings.AddSimpleScrubber("peer.service: sqledge_arm64", "peer.service: sqlserver");
settings.AddRegexScrubber(new Regex("dd.instrumentation.time_ms: \\d+.\\d+"), "dd.instrumentation.time_ms: 123.456");

var fileName = nameof(MicrosoftDataSqliteTests);

await VerifyHelper.VerifySpans(spans, settings)
.DisableRequireUniquePrefix()
.UseFileName($"{fileName}.Schema{metadataSchemaVersion.ToUpper()}");
}

[SkippableFact]
Expand Down
21 changes: 20 additions & 1 deletion tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/AdoNet/SystemDataSqliteTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,17 @@
// </copyright>

using System.Linq;
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using Datadog.Trace.Configuration;
using Datadog.Trace.TestHelpers;
using VerifyXunit;
using Xunit;
using Xunit.Abstractions;

namespace Datadog.Trace.ClrProfiler.IntegrationTests.AdoNet
{
[UsesVerify]
public class SystemDataSqliteTests : TracingIntegrationTest
{
public SystemDataSqliteTests(ITestOutputHelper output)
Expand Down Expand Up @@ -56,7 +59,7 @@ public async Task IntegrationDisabled()

private async Task RunTest(string metadataSchemaVersion)
{
const int expectedSpanCount = 91;
const int expectedSpanCount = 105;
const string dbType = "sqlite";
const string expectedOperationName = dbType + ".query";

Expand All @@ -72,6 +75,22 @@ private async Task RunTest(string metadataSchemaVersion)
Assert.Equal(expectedSpanCount, spans.Count);
ValidateIntegrationSpans(spans, metadataSchemaVersion, expectedServiceName: clientSpanServiceName, isExternalSpan);
telemetry.AssertIntegrationEnabled(IntegrationId.Sqlite);

var settings = VerifyHelper.GetSpanVerifierSettings();
settings.AddRegexScrubber(new Regex("SQLite-Test-[a-zA-Z0-9]{32}"), "System-Data-SqlClient-Test-GUID");
settings.AddSimpleScrubber("out.host: localhost", "out.host: sqlserver");
settings.AddSimpleScrubber("out.host: (localdb)\\MSSQLLocalDB", "out.host: sqlserver");
settings.AddSimpleScrubber("out.host: sqledge_arm64", "out.host: sqlserver");
settings.AddSimpleScrubber("peer.service: localhost", "peer.service: sqlserver");
settings.AddSimpleScrubber("peer.service: (localdb)\\MSSQLLocalDB", "peer.service: sqlserver");
settings.AddSimpleScrubber("peer.service: sqledge_arm64", "peer.service: sqlserver");
settings.AddRegexScrubber(new Regex("dd.instrumentation.time_ms: \\d+.\\d+"), "dd.instrumentation.time_ms: 123.456");

var fileName = nameof(SystemDataSqliteTests);

await VerifyHelper.VerifySpans(spans, settings)
.DisableRequireUniquePrefix()
.UseFileName($"{fileName}.Schema{metadataSchemaVersion.ToUpper()}");
}
}
}
4 changes: 3 additions & 1 deletion tracer/test/Datadog.Trace.Tests/DatabaseMonitoring/DatabaseMonitoringPropagatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,12 @@ public void ExpectedContextSet(string propagationMode, string integration, int s
var commandMock = new Mock<IDbCommand>();
var parameterMock = new Mock<IDbDataParameter>();
connectionMock.Setup(c => c.CreateCommand()).Returns(commandMock.Object);
connectionMock.SetupGet(c => c.State).Returns(ConnectionState.Open);
commandMock.SetupSet(c => c.CommandText = It.IsAny<string>())
.Callback<string>(value => sql = value);
commandMock.Setup(c => c.CreateParameter()).Returns(parameterMock.Object);
commandMock.SetupGet(c => c.Parameters).Returns(Mock.Of<IDataParameterCollection>());
commandMock.SetupGet(c => c.Connection).Returns(connectionMock.Object);
parameterMock.SetupSet(p => p.Value = It.IsAny<byte[]>())
.Callback<object>(value => context = (byte[])value);

Expand All @@ -156,7 +158,7 @@ public void ExpectedContextSet(string propagationMode, string integration, int s
var span = tracer.StartSpan("db.query", parent: SpanContext.None, serviceName: "pouet", traceId: new TraceId(Upper: 0xBABEBABEBABEBABE, Lower: 0xCAFECAFECAFECAFE), spanId: 0xBEEFBEEFBEEFBEEF);
span.Context.TraceContext.SetSamplingPriority(samplingPriority);

DatabaseMonitoringPropagator.PropagateDataViaContext(dbmPropagationLevel, integrationId, connectionMock.Object, span);
DatabaseMonitoringPropagator.PropagateDataViaContext(dbmPropagationLevel, integrationId, commandMock.Object, span);

if (shouldInject)
{
Expand Down
Loading
Loading