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

Changes from 1 commit
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
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 @@
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 @@ -210,6 +212,13 @@
{
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));

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net462) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net462) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net462) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net462) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (windows-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (ubuntu-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (ubuntu-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (ubuntu-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'

Check failure on line 219 in src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs

View workflow job for this annotation

GitHub Actions / build-test-instrumentation-sqlclient / build-test (ubuntu-latest, net8.0) 

'SemanticConventions' does not contain a definition for 'AttributeDbResponseStatusCode'
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
Loading