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

Sql for .net framework should call Enrich #4161

Closed
wants to merge 2 commits into from
Closed

Sql for .net framework should call Enrich #4161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
611e25f
Use Enrich in .net framework
davhdavh Feb 7, 2023
a0ef191
Update SqlClientInstrumentationOptions.cs
davhdavh Feb 7, 2023
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
9 changes: 9 additions & 0 deletions src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlEventSourceListener.netfx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,15 @@ private void OnBeginExecute(EventWrittenEventArgs eventData)
activity.SetTag(SemanticConventions.AttributeDbStatement, commandText);
}
}

try
{
this.options.Enrich?.Invoke(activity, "OnCustom", eventData.Payload);
Copy link
Member

Choose a reason for hiding this comment

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

Is payload useful to anyone? IIRC we didn't add this because it is just an array of a few strings and there was no way to get at the raw request (command object).

Copy link
Author

Choose a reason for hiding this comment

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

Nothing I am aware of, I cannot think of any reason to use the SqlCommand either, but it encapsulates the same idea as the SqlCommand that is passed from .NET in providing the raw data from with the activity is created from.

Copy link
Member

Choose a reason for hiding this comment

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

if there is no use case, then we dont need Enrich for .NET framework, right? (thats the current behavior)

Copy link
Author

Choose a reason for hiding this comment

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

Just because I cannot see a usecase for the 3rd parameter. That doesn't mean there is no usecase at all. E.g. i want the activity.Displayname to be the same no matter what the underlaying db name is.

Copy link
Member

Choose a reason for hiding this comment

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

to change activity.DisplayName, you can use ActivityProcessor. The whole Enrich feature was provided to provide easy access to raw objects from which users can extract additional information. In case of SQL on .NET FW, the raw object(payload) do not have anything not already parsed out by default.

cijothomas marked this conversation as resolved.
Show resolved Hide resolved
}
catch (Exception ex)
{
SqlClientInstrumentationEventSource.Log.EnrichmentException(ex);
}
}

private void OnEndExecute(EventWrittenEventArgs eventData)
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,14 @@ public class SqlClientInstrumentationOptions
/// raw <c>SqlCommand</c> object.
/// </summary>
/// <remarks>
/// <para><b>Enrich is only executed on .NET and .NET Core
/// <para><b>Enrich is only executed on .NET Framework and .NET
/// runtimes.</b></para>
/// The parameters passed to the enrich action are:
/// <list type="number">
/// <item>The <see cref="Activity"/> being enriched.</item>
/// <item>The name of the event. Currently only <c>"OnCustom"</c> is
/// used but more events may be added in the future.</item>
/// <item>The raw <c>SqlCommand</c> object from which additional
/// <item>The raw <c>SqlCommand</c> object (.NET) or Event payload (.NET Framework) from which additional
/// information can be extracted to enrich the <see
/// cref="Activity"/>.</item>
/// </list>
Expand Down