-
Notifications
You must be signed in to change notification settings - Fork 579
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.
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
feat(attributes) bump semconv to 1.26.0 #6172
Open
prestonvasquez
wants to merge
25
commits into
open-telemetry:main
Choose a base branch
from
prestonvasquez:otelmongo#6171
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
95348f2
otelmongo#6171 bump semconv to 1.26.0
prestonvasquez f7449ef
Merge branch 'main' into otelmongo#6171
prestonvasquez e075e01
otelmongo#6171 update changelog
prestonvasquez c863b33
Merge branch 'otelmongo#6171' of github.com:prestonvasquez/openteleme…
prestonvasquez 6b5056e
Merge branch 'main' into otelmongo#6171
prestonvasquez 3db421e
otelmongo#6171 Add semconv registry
prestonvasquez a2bc652
Merge branch 'otelmongo#6171' of github.com:prestonvasquez/openteleme…
prestonvasquez 2eef3fe
otelmongo#6171 Specify generic types
prestonvasquez aaa6129
otelmongo#6171 Remove profiling files
prestonvasquez bf676a1
Update CHANGELOG.md
prestonvasquez c5b3f3c
otelmongo#6171 clean up tests
prestonvasquez d0ec780
Merge branch 'otelmongo#6171' of github.com:prestonvasquez/openteleme…
prestonvasquez b42dac7
otelmongo#6171 Resolve merge conflicts
prestonvasquez 9aed9c9
otelmongo#6171 udpate changelog
prestonvasquez 00ad843
otelmongo#6171 use goimports to format testfile
prestonvasquez 6ebd6d2
otelmongo#6171 Run precommit
prestonvasquez 91a4e29
Remove registry pattern
prestonvasquez d7f792f
Update docs and changelog
prestonvasquez fd6c1f6
Remove version
prestonvasquez b140c55
Add license
prestonvasquez 3fd3835
Merge main
prestonvasquez 3777228
Use switch statement for tcp
prestonvasquez e9b22bd
Merge branch 'main' into otelmongo#6171
prestonvasquez 39b5893
Move CHANGELOG
prestonvasquez 6de6ad5
run precommit
prestonvasquez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
88 changes: 88 additions & 0 deletions
88
instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo/semconv.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package otelmongo // import "go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo" | ||
|
|
||
| import ( | ||
| "os" | ||
|
|
||
| "go.opentelemetry.io/otel/attribute" | ||
| semconv1210 "go.opentelemetry.io/otel/semconv/v1.21.0" | ||
| semconv1260 "go.opentelemetry.io/otel/semconv/v1.26.0" | ||
| ) | ||
|
|
||
| const ( | ||
| semconvOptIn = "OTEL_SEMCONV_STABILITY_OPT_IN" | ||
| semconvOptIn1260 = "mongo" | ||
| semconvOptInDup = "mongo/dup" | ||
| ) | ||
|
|
||
| func appendAttrs[T string | int]( | ||
| attrs []attribute.KeyValue, | ||
| semconvMap1210 func(T) attribute.KeyValue, | ||
| semconvMap1260 func(T) attribute.KeyValue, | ||
| val T, | ||
| ) []attribute.KeyValue { | ||
| switch os.Getenv(semconvOptIn) { | ||
| case semconvOptIn1260: | ||
| if semconvMap1260 != nil { | ||
| attrs = append(attrs, semconvMap1260(val)) | ||
| } | ||
| case semconvOptInDup: | ||
| if semconvMap1210 != nil { | ||
| attrs = append(attrs, semconvMap1210(val)) | ||
| } | ||
|
|
||
| if semconvMap1260 != nil { | ||
| attrs = append(attrs, semconvMap1260(val)) | ||
| } | ||
| default: | ||
| if semconvMap1210 != nil { | ||
| attrs = append(attrs, semconvMap1210(val)) | ||
| } | ||
| } | ||
|
|
||
| return attrs | ||
| } | ||
|
|
||
| func appendOpNameAttrs(attrs []attribute.KeyValue, op string) []attribute.KeyValue { | ||
| return appendAttrs(attrs, semconv1210.DBOperation, semconv1260.DBOperationName, op) | ||
| } | ||
|
|
||
| func appendDBNamespace(attrs []attribute.KeyValue, ns string) []attribute.KeyValue { | ||
| return appendAttrs(attrs, semconv1210.DBName, semconv1260.DBNamespace, ns) | ||
| } | ||
|
|
||
| func appendDBStatement(attrs []attribute.KeyValue, stmt string) []attribute.KeyValue { | ||
| return appendAttrs(attrs, semconv1210.DBStatement, semconv1260.DBQueryText, stmt) | ||
| } | ||
|
|
||
| func appendNetworkPort(attrs []attribute.KeyValue, p int) []attribute.KeyValue { | ||
| return appendAttrs(attrs, semconv1210.NetPeerPort, semconv1260.NetworkPeerPort, p) | ||
| } | ||
|
|
||
| func appendNetworkHost(attrs []attribute.KeyValue, h string) []attribute.KeyValue { | ||
| return appendAttrs(attrs, semconv1210.NetPeerName, nil, h) | ||
| } | ||
|
|
||
| func appendNetworkAddress(attrs []attribute.KeyValue, addr string) []attribute.KeyValue { | ||
| return appendAttrs(attrs, nil, semconv1260.NetworkPeerAddress, addr) | ||
| } | ||
|
|
||
| func appendNetworkTransport(attrs []attribute.KeyValue) []attribute.KeyValue { | ||
| switch os.Getenv(semconvOptIn) { | ||
| case semconvOptIn1260: | ||
| attrs = append(attrs, semconv1260.NetworkTransportTCP) | ||
| case semconvOptInDup: | ||
| attrs = append(attrs, semconv1260.NetworkTransportTCP) | ||
| fallthrough | ||
| default: | ||
| attrs = append(attrs, semconv1210.NetTransportTCP) | ||
| } | ||
|
|
||
| return attrs | ||
| } | ||
|
|
||
| func appendCollection(attrs []attribute.KeyValue, coll string) []attribute.KeyValue { | ||
| return appendAttrs(attrs, semconv1210.DBMongoDBCollection, semconv1260.DBCollectionName, coll) | ||
| } |
111 changes: 111 additions & 0 deletions
111
instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo/semconv_test.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package otelmongo | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| "go.opentelemetry.io/otel/attribute" | ||
| ) | ||
|
|
||
| func Test_appendOpNameAttrs(t *testing.T) { | ||
| const ( | ||
| opName = "opName" | ||
| dbNamespace = "dbNamespace" | ||
| port = 1 | ||
| host = "host" | ||
| address = "host:1" | ||
| stmt = `{insert: "users"}` | ||
| coll = "coll" | ||
| ) | ||
|
|
||
| v1170 := []attribute.KeyValue{ | ||
| {Key: "db.operation", Value: attribute.StringValue(opName)}, | ||
| {Key: "db.name", Value: attribute.StringValue(dbNamespace)}, | ||
| {Key: "db.statement", Value: attribute.StringValue(stmt)}, | ||
| {Key: "net.peer.port", Value: attribute.IntValue(port)}, | ||
| {Key: "net.peer.name", Value: attribute.StringValue(host)}, | ||
| {Key: "net.transport", Value: attribute.StringValue("ip_tcp")}, | ||
| {Key: "db.mongodb.collection", Value: attribute.StringValue("coll")}, | ||
| } | ||
|
|
||
| v1260 := []attribute.KeyValue{ | ||
| {Key: "db.operation.name", Value: attribute.StringValue(opName)}, | ||
| {Key: "db.namespace", Value: attribute.StringValue(dbNamespace)}, | ||
| {Key: "db.query.text", Value: attribute.StringValue(stmt)}, | ||
| {Key: "network.peer.port", Value: attribute.IntValue(port)}, | ||
| {Key: "network.peer.address", Value: attribute.StringValue(address)}, | ||
| {Key: "network.transport", Value: attribute.StringValue("tcp")}, | ||
| {Key: "db.collection.name", Value: attribute.StringValue("coll")}, | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| initAttrs []attribute.KeyValue | ||
| version string | ||
| want []attribute.KeyValue | ||
| }{ | ||
| { | ||
| name: "no version", | ||
| initAttrs: []attribute.KeyValue{}, | ||
| version: "", | ||
| want: v1170, | ||
| }, | ||
| { | ||
| name: "unsupported version", | ||
| initAttrs: []attribute.KeyValue{}, | ||
| version: "mongo/foo", | ||
| want: v1170, | ||
| }, | ||
| { | ||
| name: "mongo", | ||
| initAttrs: []attribute.KeyValue{}, | ||
| version: "mongo", | ||
| want: v1260, | ||
| }, | ||
| { | ||
| name: "mongo/dup", | ||
| initAttrs: []attribute.KeyValue{}, | ||
| version: "mongo/dup", | ||
| want: append(v1170, v1260...), | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| t.Setenv(semconvOptIn, test.version) | ||
|
|
||
| attrs := appendOpNameAttrs(test.initAttrs, opName) | ||
| attrs = appendDBNamespace(attrs, dbNamespace) | ||
| attrs = appendDBStatement(attrs, stmt) | ||
| attrs = appendNetworkPort(attrs, port) | ||
| attrs = appendNetworkHost(attrs, host) | ||
| attrs = appendNetworkAddress(attrs, address) | ||
| attrs = appendNetworkTransport(attrs) | ||
| attrs = appendCollection(attrs, coll) | ||
|
|
||
| assert.ElementsMatch(t, test.want, attrs) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func Benchmark_appendAttrs(b *testing.B) { | ||
| ini := []attribute.KeyValue{} | ||
|
|
||
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
|
|
||
| for i := 0; i < b.N; i++ { | ||
| ini = appendOpNameAttrs(ini, "opName") | ||
| ini = appendDBNamespace(ini, "dbNamespace") | ||
| ini = appendDBStatement(ini, `{insert: "users"}`) | ||
| ini = appendNetworkPort(ini, 1) | ||
| ini = appendNetworkHost(ini, "host") | ||
| ini = appendNetworkAddress(ini, "host:1") | ||
| ini = appendNetworkTransport(ini) | ||
| ini = appendCollection(ini, "coll") | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than creating one method for each attribute, how about one method which returns all attributes, and can decide their key name.
Kind of like what the
otelhttplibrary does, where we define two structs, one for the old and another for the new version.https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation/net/http/otelhttp/internal/semconv
I would also move the logic into an internal package, for a better separation of concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting idea. If I understand correctly, you're suggesting we define two separate command monitors specifically for the semantic conventions. Could you explain the benefits of this pattern over the more atomic append* solution proposed in the PR? The goal of the existing solution is to minimize code changes when advancing the library to a desired version, while also avoiding duplication in the monitoring logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your solution works if some values are renamed. But if a new value is added, or one is removed then, it becomes trickier to handle since the root method has to know about everything.
With one struct for each semconv version, that struct knows exactly what to do for that version, and that's it. There aren't two versions mixed.
It would also avoid having to define one method for each attribute.
Finally, doing the separation this way also makes it easier to add a third version if we someday need it (I really hope we don't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At last using the same patterns in multiple modules help in maintenance of the repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmathieu Here are my arguments for the factory pattern over struct duplication:
In this case you would just add
nilto the unused version,networkHostfor example:This is pretty low complexity, exchanging 1 line of code for 3 to avoid refactoring the library.
By design, there can never be a third version. The clarification of this point can be found here: open-telemetry/semantic-conventions#1502. Additionally, this logic should be rolled back entirely once we release a stable version:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prestonvasquez, please first create a PR which refactors
otelhttp. We need to try using similar patterns across the codebase to make it easier to maintain and more readable. Ideally the pattern should be described in https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#style-guide.