[SqlClient] Add error.type and db.response.status_code attributes #2262
Conversation
github-actions
bot
added
the
comp:instrumentation.sqlclient
| @@ -210,6 +212,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); | |||
There was a problem hiding this comment.
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 MyTableThis 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.
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs
Show resolved
Hide resolved
Fixes #2239