-
Notifications
You must be signed in to change notification settings - Fork 780
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.
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
Added instrumentation for netfx SqlClient. #761
Changes from 1 commit
06978ba
40dd3b1
fb25ff8
401f740
dade046
baa5f1b
14c399d
c3a499c
83200ea
7e62d1b
b9908b8
e9ac05e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,6 @@ | |
using System.Data; | ||
using System.Diagnostics; | ||
using OpenTelemetry.Trace; | ||
using OpenTelemetry.Trace.Samplers; | ||
|
||
namespace OpenTelemetry.Instrumentation.Dependencies.Implementation | ||
{ | ||
|
@@ -91,11 +90,10 @@ public override void OnCustom(string name, Activity activity, object payload) | |
|
||
if (this.commandTypeFetcher.Fetch(command) is CommandType commandType) | ||
{ | ||
activity.AddTag(DatabaseStatementTypeSpanAttributeKey, commandType.ToString()); | ||
|
||
switch (commandType) | ||
{ | ||
case CommandType.StoredProcedure: | ||
activity.AddTag(DatabaseStatementTypeSpanAttributeKey, "StoredProcedure"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using the magic string "StoredProcedure", i would use nameof(commandtype) or nameof(CommandType.StoredProcedure). The same comment applys for "Text" / "TableDirect" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md Nothing here about "Type"! should we raise an issue to add it to spec? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eddynaka What you have against magic? j/k I added the @cijothomas I'll open an issue in spec for
|
||
if (this.options.CaptureStoredProcedureCommandName) | ||
{ | ||
activity.AddTag(SpanAttributeConstants.DatabaseStatementKey, (string)commandText); | ||
|
@@ -104,12 +102,17 @@ public override void OnCustom(string name, Activity activity, object payload) | |
break; | ||
|
||
case CommandType.Text: | ||
activity.AddTag(DatabaseStatementTypeSpanAttributeKey, "Text"); | ||
CodeBlanch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (this.options.CaptureTextCommandContent) | ||
{ | ||
activity.AddTag(SpanAttributeConstants.DatabaseStatementKey, (string)commandText); | ||
} | ||
|
||
break; | ||
|
||
case CommandType.TableDirect: | ||
activity.AddTag(DatabaseStatementTypeSpanAttributeKey, "TableDirect"); | ||
break; | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
// <copyright file="SqlEventSourceListener.netfx.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
#if NETFRAMEWORK | ||
using System; | ||
using System.Data.SqlClient; | ||
using System.Diagnostics; | ||
using System.Diagnostics.Tracing; | ||
using System.Reflection; | ||
using OpenTelemetry.Trace; | ||
|
||
namespace OpenTelemetry.Instrumentation.Dependencies.Implementation | ||
{ | ||
/// <summary> | ||
/// .NET Framework SqlClient doesn't emit DiagnosticSource events. | ||
/// We hook into its EventSource if it is available: | ||
/// See: <a href="https://github.com/microsoft/referencesource/blob/3b1eaf5203992df69de44c783a3eda37d3d4cd10/System.Data/System/Data/Common/SqlEventSource.cs#L29">reference source</a>. | ||
/// </summary> | ||
internal class SqlEventSourceListener : EventListener | ||
{ | ||
internal const string ActivitySourceName = "SqlClient"; | ||
CodeBlanch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
internal const string ActivityName = ActivitySourceName + ".Execute"; | ||
|
||
private static readonly Version Version = typeof(SqlEventSourceListener).Assembly.GetName().Version; | ||
private static readonly ActivitySource SqlClientActivitySource = new ActivitySource(ActivitySourceName, Version.ToString()); | ||
private static readonly EventSource SqlEventSource = (EventSource)typeof(SqlConnection).Assembly.GetType("System.Data.SqlEventSource")?.GetField("Log", BindingFlags.Static | BindingFlags.NonPublic)?.GetValue(null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we make a EventSource writer (i.e FakeSql : EventSource) which mimics the SqlClient event, we could unit test, right? (similar to unit tests firing DiagnosticSource events?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cijothomas I'll investigate this docker thing a bit. If it's quick to get up and running that would be great. Otherwise, I'll add some "faked" tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to get integration tests up and running on docker but ran into a problem. This is .NET Framework specific code, so we need a Windows container. That's not supported on GitHub at the moment, from what I can tell (ref). In the end, I did two things:
I did get pretty far into getting the container up and running. What I would like to do is complete this PR, and then I can try to get Sql integration tests up and running for our .NET Core stuff? I think that will work because there are SQL Linux images and of course .NET Core SDK Linux images. |
||
|
||
private readonly SqlClientInstrumentationOptions options; | ||
|
||
public SqlEventSourceListener(SqlClientInstrumentationOptions options = null) | ||
{ | ||
this.options = options ?? new SqlClientInstrumentationOptions(); | ||
|
||
if (SqlEventSource != null) | ||
CodeBlanch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
this.EnableEvents(SqlEventSource, EventLevel.Informational); | ||
} | ||
} | ||
|
||
public override void Dispose() | ||
{ | ||
if (SqlEventSource != null) | ||
{ | ||
this.DisableEvents(SqlEventSource); | ||
} | ||
|
||
base.Dispose(); | ||
} | ||
|
||
protected override void OnEventWritten(EventWrittenEventArgs eventData) | ||
{ | ||
if (eventData.EventId == 1) | ||
CodeBlanch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
// BeginExecuteEventId | ||
|
||
var activity = SqlClientActivitySource.StartActivity(ActivityName, ActivityKind.Client); | ||
CodeBlanch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (activity == null) | ||
{ | ||
return; | ||
} | ||
|
||
activity.DisplayName = (string)eventData.Payload[2]; | ||
|
||
if (activity.IsAllDataRequested) | ||
{ | ||
activity.AddTag(SpanAttributeConstants.ComponentKey, "sql"); | ||
|
||
activity.AddTag(SpanAttributeConstants.DatabaseTypeKey, "sql"); | ||
activity.AddTag(SpanAttributeConstants.PeerServiceKey, (string)eventData.Payload[1]); | ||
activity.AddTag(SpanAttributeConstants.DatabaseInstanceKey, (string)eventData.Payload[2]); | ||
|
||
if (string.IsNullOrEmpty((string)eventData.Payload[3])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this throw if Payload has less than 4 count? Lets do a safety check to ensure payload[3] is fine. (some old version did not provide all the payload , maybe irrelevant now, but lets be on defensive side) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added some defensive checks. Now validates for payload being null, payload not being the expected size. Doesn't validate the types of the payload so casts could blow up. But that seemed like taking it a step too far? LMK |
||
{ | ||
activity.AddTag(SpanAttributeConstants.DatabaseStatementKey, "Text"); | ||
} | ||
else | ||
{ | ||
activity.AddTag(SpanAttributeConstants.DatabaseStatementKey, "StoredProcedure"); | ||
if (this.options.CaptureStoredProcedureCommandName) | ||
{ | ||
activity.AddTag(SpanAttributeConstants.DatabaseStatementKey, (string)eventData.Payload[3]); | ||
} | ||
} | ||
} | ||
} | ||
else if (eventData.EventId == 2) | ||
{ | ||
// EndExecuteEventId | ||
|
||
var activity = Activity.Current; | ||
if (activity == null || activity.Source != SqlClientActivitySource) | ||
{ | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we dont ever expect to this condition right? I'd suggest to add a Warning/Error log. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't this happen for samplers that return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes you are right. This part is still not expected - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my mind, it's expected. That case would be, sampler returned Activity myActivity = new Activity("Call SQL user activity");
myActivity.Start();
try
{
using (SqlConnection connection = new SqlConnection("Data Source=sqlserver;Initial Catalog=master;Integrated Security=True;"))
{
connection.Open();
using (SqlCommand command = new SqlCommand("select 1/1", connection))
{
command.ExecuteNonQuery();
}
}
}
catch { }
myActivity.Stop();
Log.LogActivity(myActivity); In that case, when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perfect! thanks for clarifying. |
||
} | ||
|
||
try | ||
{ | ||
if (activity.IsAllDataRequested) | ||
{ | ||
if (((int)eventData.Payload[1] & 0x01) == 0x00) | ||
{ | ||
activity.AddTag(SpanAttributeConstants.StatusCodeKey, SpanHelper.GetCachedCanonicalCodeString(StatusCanonicalCode.Unknown)); | ||
activity.AddTag(SpanAttributeConstants.StatusDescriptionKey, $"SqlExceptionNumber {eventData.Payload[2]} thrown."); | ||
} | ||
else | ||
{ | ||
activity.AddTag(SpanAttributeConstants.StatusCodeKey, SpanHelper.GetCachedCanonicalCodeString(StatusCanonicalCode.Ok)); | ||
} | ||
} | ||
} | ||
finally | ||
{ | ||
activity.Stop(); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was made to avoid the
.ToString
perf hit. Unrelated to the main changes, sorry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D I read this comment late, and was still trying to figure out this part !