-
Notifications
You must be signed in to change notification settings - Fork 183
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
rename some code attributes #1624
Conversation
One problem is that renaming with a prefix is not allowed by policy, as a result I get the following error when trying to validate policy:
I don't know where does this restriction comes from, is it related to the generated per-language bindings ? From what I've seen so far they are mostly used as strings, so the limitations would be more on the processing/storage side. |
After digging a bit further this kind of limitation comes from the generated code par, where in some cases it could create conflicts, for example It seems that #1462 could provide a solution by allowing to provide an explicit code friendly name (which we can ensure does not conflicts), but I definitely lack expertise to know if it would help solve this current issue. @lmolkova do you think this would be covered by the current proposal ? |
We can add exceptions here: https://github.com/open-telemetry/semantic-conventions/blob/main/policies/attribute_name_collisions.rego#L83 This rule is in place to avoid conflicts if we ever "un-flatten" attributes. E.g. if an attribute of We should ensure we never send the attribute as a namespace and the new attribute in the same signal. @lmolkova will update policy to avoid this warning if the namespace attribute becomes deprecated. |
… into code-rename
Thanks to #1642 this is now unblocked and all the checks are now passing, it should now be ready for review. |
Follow semantic conventions more strictly. See the following thread: open-telemetry/semantic-conventions#1624 (comment) Follows #6253 There is no noticeable performance overhead: ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/contrib/bridges/otelslog cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ Handler/(WithSource).Handle-16 260.4n ± 21% 234.0n ± 5% -10.14% (p=0.035 n=10) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ Handler/(WithSource).Handle-16 248.0 ± 0% 248.0 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ Handler/(WithSource).Handle-16 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal ```
Today, I encountered a potential unclarity related to For instance, in Go we can have an anonymous function in
I started to have doubts whether the split |
As a reminder, the current state of this PR renames the following fields, there has been some approvals before the
There is only one unresolved conversation: #1624 (comment), which is mostly about introducing a breaking change, @trask @lmolkova what do you think are the best next steps ? Do you think it's worth doing a proper migration plan ? |
@SylvainJuge let's follow @trask's proposal in #1624 (comment) to add a warning - this would reduce any additional churn (if any). Instrumentations that emit existing |
I've added a warning (and a dedicated |
… into code-rename
… into code-rename
Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
… into code-rename
Relates to #1377
In #1599 PR discussion, it was suggested we should probably rename a few
code.*
attributes before attempting to make them stable (which is the goal of #1599).Changes
code.function
tocode.function.name
: it allows to add in the future other function related attributes later likecode.function.visibility
orcode.function.signature
.code.lineno
tocode.line.number
this follows the general recommendation to avoid abbreviations while thelineno
term is common, avoiding ambiguity is probably better here, also thecode.line
might be ambiguous as it could be interpreted as "the line of code".code.column
tocode.column.number
for consistency/symmetry withcode.line.number
.code.filepath
tocode.file.path
for consistency withfile.path
(comment)Existing usages of those attributes would be impacted (from a GH search, which means some might have been missed due to indirections from the respective generated semconv files):
opentelemetry-php
code.lineno
searchcode.function
searchopentelemetry-rust
code.lineno
searchopentelemetry-java-instrumentation
code.lineno
searchcode.function
searchopentelemetry-js-contrib
code.lineno
searchcode.function
searchopentelemetry-python
code.lineno
searchcode.function
searchopentelemetry-cpp-contrib
code.function
searchcode.lineno
searchopentelemetry-php-contrib
code.lineno
searchcode.function
searchI haven't found any usage of
code.column
which means renaming here should not have any expected impact.EDIT Renaming
code.filepath
tocode.file.path
has been added later in the discussion and potential impacts haven't been evaluated.Merge requirement checklist
[chore]