update HttpSemanticConventions for Instrumentation.SqlClient - #4644
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4644 +/- ##
==========================================
+ Coverage 84.94% 84.98% +0.04%
==========================================
Files 314 314
Lines 12689 12698 +9
==========================================
+ Hits 10779 10792 +13
+ Misses 1910 1906 -4
|
Co-authored-by: Vishwesh Bankwar <vishweshbankwar@users.noreply.github.com>
vishweshbankwar
left a comment
vishweshbankwar
left a comment
There was a problem hiding this comment.
LGTM - Please add the tests as well
| sqlActivity.SetTag(SemanticConventions.AttributeNetPeerName, connectionDetails.ServerHostName); | ||
| } | ||
| else | ||
| if (!string.IsNullOrEmpty(connectionDetails.InstanceName)) |
There was a problem hiding this comment.
Not related to this PR, but why is this method in the options class instead of the listener?
We should probably move this code to the listener. That would also make us consistent with other instrumentation libraries for how we read the environment variable.
utpilla
left a comment
utpilla
left a comment
There was a problem hiding this comment.
Made some suggestions to update the tests.
| Assert.Equal(expectedPort, activity.GetTagValue(SemanticConventions.AttributeServerPort)); | ||
| } | ||
|
|
||
| // Tests for newer HTTP v1.21.0 Semantic Conventions and older attributes. |
There was a problem hiding this comment.
It seems we still have confusion here, what exactly is "HTTP v1.21.0 Semantic Conventions", @TimothyMothra could you put a concrete link?
There was a problem hiding this comment.
Corrected this. New text:
// Tests for v1.21.0 Semantic Conventions for database client calls.
// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/database.md
// This test emits both the new and older attributes.
// This test method can be deleted when this library is GA.
|
|
||
| ## Unreleased | ||
|
|
||
| * Updated [Semantic Conventions](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/trace/semantic_conventions/database.md). |
There was a problem hiding this comment.
I think we're lacking clarity here, https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/CHANGELOG.md#v1210-2023-05-09 clearly called out that semconv got moved to a new repo so it doesn't seem to be part of v1.21.0 release, yet we're referring to 1.21.0 database link, what does that mean and what exactly are we trying to follow?
There was a problem hiding this comment.
This was already discussed. #4538 (comment)
The semantic-conventions repo doesn't have any versioned releases for me to link to.
@utpilla asked me to link to the former repo which does have versioned tags to link to.
There was a problem hiding this comment.
Let me change the question, which exact semantic convention version do we want to implement in this PR? (I don't think the goal to implement the link that you put here)
There was a problem hiding this comment.
We want to implement the breaking changes to the HTTP Semantic Conventions.
These changes are described in the v1.21.0 of the opentelemetry-specification.
https://github.com/open-telemetry/opentelemetry-specification/tree/v1.21.0
There was a problem hiding this comment.
FYI for all persons here.
Semantic-Conventions repo is readying their first release. The version will be v1.21.0. No ETA because they're building out the processes.
https://github.com/open-telemetry/semantic-conventions/blob/df8e53054147acf49918582bd446d0f524170071/CHANGELOG.md
Trask advised that we should link to "main" for now.
I need some time to update all the comments and links. Will need to create a new PR to correct everything that merged already.
There was a problem hiding this comment.
No ETA because they're building out the processes.
reyang
left a comment
reyang
left a comment
There was a problem hiding this comment.
Need to understand what this PR is trying to follow https://github.com/open-telemetry/opentelemetry-dotnet/pull/4644/files#r1261585780.
Unblocking the PR, please follow up and bring clarity
|
I've updated all the comments and changelog with the links to the correct spec (in the new repo). |
4 participants
Design discussion issue #4484
Changes
Instrumentation.SqlClientSqlClientInstrumentationOptionsto emit new attributes.SqlClientInstrumentationOptions_EnableConnectionLevelAttributes_New()to test the new attributes.
When this library is GA, this method will replace
SqlClientInstrumentationOptions_EnableConnectionLevelAttributes()SqlClientInstrumentationOptions_EnableConnectionLevelAttributes_Dupe()to test the scenario when this library emits both new and old attributes.
When this library is GA, this method can be removed.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes