Skip to content

[sql] Implement updates to new semantic conventions for stored procedures #2693

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

Merged
Kielek merged 5 commits into from
May 5, 2025
Merged

[sql] Implement updates to new semantic conventions for stored procedures #2693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c4d43f
Add db.stored_procedure.name attribute
alanwest Apr 11, 2025
ae1b39c
Update changelog
alanwest Apr 11, 2025
71100dd
Update changelog
alanwest Apr 11, 2025
8013e00
Merge branch 'main' into alanwest/db-stored-procedure-name
Kielek Apr 30, 2025
c61d3bc
Merge branch 'main' into alanwest/db-stored-procedure-name
Kielek May 5, 2025
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
6 changes: 6 additions & 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 @@ -6,6 +6,12 @@
connection's `DataSource` when used to populate the `server.address`
attribute.
([#2674](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2674))
* Updates have been made to adhere to recent changes in the new semantic
conventions. These affect you if you have opted in to the new conventions.
When `CommandType` is `StoredProcedure`, the `db.stored_procedure.name` has
been added and the `db.query.text`, `db.operation.name`, and
`db.collection.name` attributes have been removed.
([#2693](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2693))

* Updated OpenTelemetry core component version(s) to `1.12.0`.
([#2725](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2725))
Expand Down
3 changes: 1 addition & 2 deletions src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlActivitySourceHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,9 @@ internal sealed class SqlActivitySourceHelper
[
SemanticConventions.AttributeDbSystem,
SemanticConventions.AttributeDbSystemName,
SemanticConventions.AttributeDbCollectionName,
SemanticConventions.AttributeDbNamespace,
SemanticConventions.AttributeDbStoredProcedureName,
SemanticConventions.AttributeDbResponseStatusCode,
SemanticConventions.AttributeDbOperationName,
SemanticConventions.AttributeErrorType,
SemanticConventions.AttributeServerPort,
SemanticConventions.AttributeServerAddress,
Expand Down
7 changes: 2 additions & 5 deletions src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,7 @@ public override void OnEventWritten(string name, object? payload)

if (options.EmitNewAttributes)
{
activity.SetTag(SemanticConventions.AttributeDbOperationName, "EXECUTE");
activity.SetTag(SemanticConventions.AttributeDbCollectionName, commandText);
activity.SetTag(SemanticConventions.AttributeDbQueryText, commandText);
activity.SetTag(SemanticConventions.AttributeDbStoredProcedureName, commandText);
Copy link
Copy Markdown
Member Author

@alanwest alanwest Apr 11, 2025

Choose a reason for hiding this comment

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

@trask @lmolkova @maryliag

Wanted to check with you all to confirm this meets the spirt of the recent changes regarding stored procedures.

  1. Removing db.operation.name. I was arbitrarily setting it to EXECUTE before since there was no "operation" present in the query text.
  2. Removing db.collection.name and db.query.text since their values would simply be the same as db.stored_procedure.name making them redundant.

Copy link
Copy Markdown
Member Author

@alanwest alanwest Apr 11, 2025

Choose a reason for hiding this comment

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

Also, once I add db.query.summary should db.query.summary = db.stored_procedure.name?

Copy link
Copy Markdown
Member

@trask trask Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

db.collection.name shouldn't be captured since it's not a collection

I would expect db.query.text to be captured if the stored proc is run via a query API (where you pass SQL), but from a non-query API where you explicitly pass a stored proc name

db.operation.name is a good question. I think if it's not a query API (where you pass SQL), then we should capture db.operation.name (but I suspect spec may not be clear about this)

Copy link
Copy Markdown
Member

@trask trask Apr 11, 2025

Choose a reason for hiding this comment

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

I don't think Java has a non query API for calling stored procedures

https://docs.oracle.com/javase/8/docs/api/java/sql/Connection.html#prepareCall-java.lang.String-

Copy link
Copy Markdown
Member Author

@alanwest alanwest Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The difference between using the "non-query" vs "query" API when invoking a stored procedure in C# is subtle.

"Non-query API"

This will be the more common way to invoke a stored procedure.

var command = new SqlCommand();
command.CommandType = CommandType.StoredProcedure;
command.CommandText = "some_stored_procedure"
command.Parameters.AddWithValue("@some_parameter", "some value");

This is what I think you're proposing:

db.stored_procedure.name = some_stored_procedure
db.operation.name = EXECUTE or CALL or EXEC or ... maybe this is where the spec could be more clear
db.query.summary = EXECUTE some_stored_procedure (or CALL or EXEC)
db.query.text is not set

"Query API"

This will likely be less common, but still possible.

var command = new SqlCommand();
command.CommandType = CommandType.Text;
command.CommandText = "some_stored_procedure @some_parameter"
command.Parameters.AddWithValue("@some_parameter", "some value");

I guess in this scenario we might only be able to set db.query.text.

db.stored_procedure.name is not set
db.operation.name is not set
db.query.text = some_stored_procedure @some_parameter
db.query.summary = ??? (it might be possible to parse the query text and safely infer that it is likely a stored procedure)

Copy link
Copy Markdown
Member

@trask trask May 1, 2025

Choose a reason for hiding this comment

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

thanks

In this case, should db.query.summary be some_stored_procedure or should it be exec some_stored_procedure?

I'm thinking it should be some_stored_procedure based on:

Instrumentations that support query parsing SHOULD parse the query and extract a
list of operations and targets from the query. It SHOULD set db.query.summary
attribute to the value formatted in the following way:

{operation1} {target1} {operation2} {target2} {target3} ...

Copy link
Copy Markdown
Member

@trask trask May 1, 2025

Choose a reason for hiding this comment

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

  • not filed yet: add operation and collection name to SQL

now filed: open-telemetry/semantic-conventions#2207

Copy link
Copy Markdown
Member

@lmolkova lmolkova May 1, 2025

Choose a reason for hiding this comment

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

I'm thinking it should be some_stored_procedure based on:

hmmm. but the query sent to DB would be EXEC some_stored_procedure and operation.name would be EXEC, so summary should be EXEC some_stored_procedure.

Copy link
Copy Markdown
Member

@trask trask May 1, 2025

Choose a reason for hiding this comment

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

but the query sent to DB would be EXEC some_stored_procedure

the way I understood the discussion above is that in this case:

var command = new SqlCommand();
command.CommandType = CommandType.Text;
command.CommandText = "some_stored_procedure"

the query sent to the DB would just be some_stored_procedure

Copy link
Copy Markdown
Member Author

@alanwest alanwest May 1, 2025

Choose a reason for hiding this comment

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

the query sent to the DB would just be some_stored_procedure

Yup, that's correct.

}

break;
Expand Down Expand Up @@ -278,8 +276,7 @@ private void RecordDuration(Activity? activity, object? payload, bool hasError =
{
if (this.commandTextFetcher.TryFetch(command, out var commandText))
{
tags.Add(SemanticConventions.AttributeDbOperationName, "EXECUTE");
tags.Add(SemanticConventions.AttributeDbCollectionName, commandText);
tags.Add(SemanticConventions.AttributeDbStoredProcedureName, commandText);
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/Shared/SemanticConventions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,12 @@ internal static class SemanticConventions
// New database conventions:
// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database
public const string AttributeDbSystemName = "db.system.name";
public const string AttributeDbCollectionName = "db.collection.name";
public const string AttributeDbNamespace = "db.namespace";
public const string AttributeDbOperationName = "db.operation.name";
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";
public const string AttributeDbStoredProcedureName = "db.stored_procedure.name";

#pragma warning restore CS1591 // Missing XML comment for publicly visible type or member
}
4 changes: 1 addition & 3 deletions test/OpenTelemetry.Instrumentation.SqlClient.Tests/SqlClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,9 +491,7 @@ internal static void VerifyActivityData(

if (emitNewAttributes)
{
Assert.Equal("EXECUTE", activity.GetTagValue(SemanticConventions.AttributeDbOperationName));
Assert.Equal(commandText, activity.GetTagValue(SemanticConventions.AttributeDbCollectionName));
Assert.Equal(commandText, activity.GetTagValue(SemanticConventions.AttributeDbQueryText));
Assert.Equal(commandText, activity.GetTagValue(SemanticConventions.AttributeDbStoredProcedureName));
}

break;
Expand Down
Loading