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

[SqlClient] Add error.type and db.response.status_code attributes #2262

Merged
merged 6 commits into from
Oct 31, 2024
Merged

[SqlClient] Add error.type and db.response.status_code attributes #2262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77c90d4
Add error.type and db.response.status_code attributes
alanwest Oct 28, 2024
7cc7578
Merge remote-tracking branch 'upstream/main' into alanwest/sql-errors
alanwest Oct 30, 2024
b6e9cff
Fix
alanwest Oct 30, 2024
c35133f
.NET Framework
alanwest Oct 30, 2024
35f20b7
Update changelog
alanwest Oct 30, 2024
43bc9c4
Merge branch 'main' into alanwest/sql-errors
alanwest Oct 31, 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
1 change: 1 addition & 0 deletions src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
[spans](https://github.com/open-telemetry/semantic-conventions/blob/v1.28.0/docs/database/database-spans.md).
([#2229](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2229),
[#2277](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2277),
[#2262](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2262),
[#2279](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2279))
* **Breaking change**: The `peer.service` and `server.socket.address` attributes
are no longer emitted. Users should rely on the `server.address` attribute
Expand Down
11 changes: 10 additions & 1 deletion src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@
#if !NETFRAMEWORK
using System.Data;
using System.Diagnostics;
using OpenTelemetry.Trace;
#if NET
using System.Diagnostics.CodeAnalysis;
#endif
using System.Globalization;
using OpenTelemetry.Trace;

namespace OpenTelemetry.Instrumentation.SqlClient.Implementation;

Expand All @@ -32,6 +33,7 @@ internal sealed class SqlClientDiagnosticListener : ListenerHandler
private readonly PropertyFetcher<CommandType> commandTypeFetcher = new("CommandType");
private readonly PropertyFetcher<object> commandTextFetcher = new("CommandText");
private readonly PropertyFetcher<Exception> exceptionFetcher = new("Exception");
private readonly PropertyFetcher<int> exceptionNumberFetcher = new("Number");
private readonly SqlClientTraceInstrumentationOptions options;

public SqlClientDiagnosticListener(string sourceName, SqlClientTraceInstrumentationOptions? options)
Expand Down Expand Up @@ -189,6 +191,13 @@ public override void OnEventWritten(string name, object? payload)
{
if (this.exceptionFetcher.TryFetch(payload, out Exception? exception) && exception != null)
{
activity.AddTag(SemanticConventions.AttributeErrorType, exception.GetType().FullName);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not very useful because error.type will always equal Microsoft.Data.SqlClient.SqlException.

Per the conventions, this is not necessarily wrong, but it's not ideal:

The error.type SHOULD match the db.response.status_code returned by the database or the client library, or the canonical name of exception that occurred. When using canonical exception type name, instrumentation SHOULD do the best effort to report the most relevant type. For example, if the original exception is wrapped into a generic one, the original exception SHOULD be preferred. Instrumentations SHOULD document how error.type is populated.

Doing something like the following would be more useful and would align with the response status code.

activity.AddTag(SemanticConventions.AttributeErrorType, exception.Message);

However exception message can have variable text and could cause error.type to be high cardinality. For example:

SELECT ColumnThatDoesNotExist FROM MyTable

This query results in error code 207 and message:

Invalid column name 'ColumnThatDoesNotExist'

It's tempting to do something like the following to replace anything in single-quotes with something.

Regex.Replace(exception.Message, "'[^']*'", "'%'"));

But, glancing over the list of error codes, I'm not sure such a simple replacement will suffice.

Alternatively, we could maintain a dictionary of codes to messages... there are a lot.

Copy link
Contributor

@lmolkova lmolkova Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe there is a common practice to set exception message on the span description.

It's ok if for .NET it's uncommon to subclass exceptions and that error.type won't be super-interesting. We still have status code.


if (this.exceptionNumberFetcher.TryFetch(exception, out var exceptionNumber))
{
activity.AddTag(SemanticConventions.AttributeDbResponseStatusCode, exceptionNumber.ToString(CultureInfo.InvariantCulture));
Copy link
Member Author

Choose a reason for hiding this comment

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

The error has a number, severity, and state.

The number is most descriptive of the problem that occurred, though I can see severity and state being useful.

From the docs

Severity:

The severity indicates how serious the error is. Errors that have a low severity, such as 1 or 2, are information messages or low-level warnings. Errors that have a high severity indicate problems that should be addressed as soon as possible. For more information about severities, see Database Engine Error Severities.

State:

Some error messages can be raised at multiple points in the code for the Database Engine. For example, an 1105 error can be raised for several different conditions. Each specific condition that raises an error assigns a unique state code.

When you are viewing databases that contain information about known issues, such as the Microsoft Knowledge Base, you can use the state number to determine whether the recorded issue is the same as the error you have encountered. For example, if a Knowledge Base Article describes an 1105 error that has a state of 2 and the 1105 error message you received had a state of 3, the error probably has a different cause than the one reported in the article.

A Microsoft support engineer can also use the state code from an error to find the location in the source code where that error code is being raised. This information might provide additional ideas on how to diagnose the problem.

Copy link
Contributor

@lmolkova lmolkova Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

could you please check out https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/sql.md?plain=1#L102-L136 ?

I believe MS SQL server doesn't support SQL STATE, but if you ever see both - state and code, you can concat them together - see the link above.

If severity is interesting, we can always add it later, but it seems there is a 1:1 mapping from the error code to severity and it might be redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

The notion of state here is different from SQL STATE. From what I can tell you're correct that MSSQL does not support SQL STATE.

My read is that for a given MSSQL error code, that code may be raised from different locations in the DBMS and the state specifies exactly which location the code was raised. I think I'm going to just ignore state.

If severity is interesting, we can always add it later, but it seems there is a 1:1 mapping from the error code to severity and it might be redundant.

I agree it's probably redundant.

}

activity.SetStatus(ActivityStatusCode.Error, exception.Message);

if (this.options.RecordException)
alanwest marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
10 changes: 8 additions & 2 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 @@ -175,8 +175,14 @@ private void OnEndExecute(EventWrittenEventArgs eventData)
{
if ((compositeState & 0b010) == 0b010)
{
var errorText = $"SqlExceptionNumber {eventData.Payload[2]} thrown.";
activity.SetStatus(ActivityStatusCode.Error, errorText);
var errorNumber = $"{eventData.Payload[2]}";
activity.SetStatus(ActivityStatusCode.Error, errorNumber);
activity.SetTag(SemanticConventions.AttributeDbResponseStatusCode, errorNumber);

var exceptionType = eventData.EventSource.Name == MdsEventSourceName
? "Microsoft.Data.SqlClient.SqlException"
: "System.Data.SqlClient.SqlException";
activity.SetTag(SemanticConventions.AttributeErrorType, exceptionType);
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/Shared/SemanticConventions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ internal static class SemanticConventions
public const string AttributeDbCollectionName = "db.collection.name";
public const string AttributeDbNamespace = "db.namespace";
public const string AttributeDbOperationName = "db.operation.name";
public const string AttributeResponseStatusCode = "db.response.status_code";
public const string AttributeDbResponseStatusCode = "db.response.status_code";
public const string AttributeDbOperationBatchSize = "db.operation.batch.size";
public const string AttributeDbQuerySummary = "db.query.summary";
public const string AttributeDbQueryText = "db.query.text";
Expand Down
17 changes: 17 additions & 0 deletions test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientIntegrationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,23 @@ public void SuccessfulCommandTest(

SqlClientTests.VerifyActivityData(commandType, commandText, captureStoredProcedureCommandName, captureTextCommandContent, isFailure, recordException, shouldEnrich, activity);
SqlClientTests.VerifySamplingParameters(sampler.LatestSamplingParameters);

if (isFailure)
{
#if NET
var status = activity.GetStatus();
Assert.Equal(ActivityStatusCode.Error, activity.Status);
Assert.Equal("Divide by zero error encountered.", activity.StatusDescription);
Assert.EndsWith("SqlException", activity.GetTagValue(SemanticConventions.AttributeErrorType) as string);
Assert.Equal("8134", activity.GetTagValue(SemanticConventions.AttributeDbResponseStatusCode));
#else
var status = activity.GetStatus();
Assert.Equal(ActivityStatusCode.Error, activity.Status);
Assert.Equal("8134", activity.StatusDescription);
Assert.EndsWith("SqlException", activity.GetTagValue(SemanticConventions.AttributeErrorType) as string);
Assert.Equal("8134", activity.GetTagValue(SemanticConventions.AttributeDbResponseStatusCode));
#endif
}
}

private string GetConnectionString()
Expand Down
Loading