[connector/spanmetrics] Use latest semantic conventions for status code attribute#43000
Conversation
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
be23e17 to
ad9ec76
Compare
| # Use this changelog template to create an entry for release notes. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: enhancement |
There was a problem hiding this comment.
I believe it's a breaking change
| component: spanmetricsconnector | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: Add a feature gate to use the latest semantic conventions for the status code attribute on generated metrics. |
There was a problem hiding this comment.
nit: We can use subtext to explain the rationale behind introducing this breaking change, as well as how to use feature gates to roll it back.
In note we explicitly say that the key of span status change from status.code to otel.status_code
| if useOtelStatusCodeAttribute.IsEnabled() { | ||
| if !contains(p.config.ExcludeDimensions, otelStatusCodeKey) { | ||
| if span.Status().Code() == ptrace.StatusCodeError { | ||
| attr.PutStr(otelStatusCodeKey, "ERROR") |
There was a problem hiding this comment.
This change also needs to be included in the changelog
Frapschen
left a comment
There was a problem hiding this comment.
LGTM with minor comments.
|
Please address the changes and CI and mark ready for review again. |
ad9ec76 to
629b4b6
Compare
|
@iblancasa would you mind reviewing as well? It looks like github is requiring two maintainer approvals |
629b4b6 to
50082dd
Compare
|
I think I misunderstood the issue; it seems like the workflows themselves need to be approved by a maintainer in order to run. @Frapschen would you be able to help with that? Or otherwise instruct on how to go about merging this if that isn't the blocker? Thank you! |
c0a8ab3 to
95dadeb
Compare
|
I'm not sure why the module check had failed. I rebased in hopes that it will solve the issue, but please let me know if it's otherwise actionable! I fixed the lint errors and the command the CI system had run: |
|
|
ab6d52b to
76ded24
Compare
|
Rebasing broke my new unit test since |
|
@Machyne, I have tried to find an approver to approve the workflow to run. |
|
@Machyne Please resolve the conflicts |
76ded24 to
0fff84c
Compare
|
@Frapschen I rebased and pushed again! |
|
@Machyne Changelog check fail |
0fff84c to
9f6f956
Compare
|
@Frapschen sorry about that; the check must have been changed after I rebased. It's passing locally now. |
| "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil" | ||
| ) | ||
|
|
||
| var useOtelStatusCodeAttribute = featuregate.GlobalRegistry().MustRegister( |
There was a problem hiding this comment.
@Frapschen I made this update. Sorry for the delay, I had missed your message. I believe the prior windows failure was unrelated to my change. I ran the test and lint locally after my recent rebase and I'm hoping we can get this merged!
9f6f956 to
2b6f797
Compare
2b6f797 to
4fd36dc
Compare
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
|
Thank you for your contribution @Machyne! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
#### Description - Migrate programatically registered feature gates to `metadata.yaml`. Kept comments/TODO intact as I assumed they were committed for a reason in the first place. - `from_version` for each gate was determined via `git describe --contains <commit>` on the commit that introduced it: - `connector.spanmetrics.legacyMetricNames` → `v0.109.0` (#34485) - `connector.spanmetrics.includeCollectorInstanceID` → `v0.136.0` (#41865) - `connector.spanmetrics.useSecondAsDefaultMetricsUnit` → `v0.137.0` (#42761) - `connector.spanmetrics.excludeResourceMetrics` → `v0.137.0` (#42760) - `spanmetrics.statusCodeConvention.useOtelPrefix` → `v0.141.0` (#43000) - `spanmetrics.statusCodeConvention.useOtelPrefix` had no `reference_url` in the original registration, so I used the implementing PR as the reference URL as the issue was only partially fixed. - `connector_test.go` already imports `google.golang.org/grpc/metadata`, so the internal `metadata` package was imported with the alias `spanmetricsmetadata` to avoid a name conflict #### Link to tracking issue Part of #46116 #### Testing - `make` in the component directory - `make gogci` in root dir #### Documentation - `connector/spanmetricsconnector/documentation.md` was generated via `make generate`
…ry#48281) #### Description - Migrate programatically registered feature gates to `metadata.yaml`. Kept comments/TODO intact as I assumed they were committed for a reason in the first place. - `from_version` for each gate was determined via `git describe --contains <commit>` on the commit that introduced it: - `connector.spanmetrics.legacyMetricNames` → `v0.109.0` (open-telemetry#34485) - `connector.spanmetrics.includeCollectorInstanceID` → `v0.136.0` (open-telemetry#41865) - `connector.spanmetrics.useSecondAsDefaultMetricsUnit` → `v0.137.0` (open-telemetry#42761) - `connector.spanmetrics.excludeResourceMetrics` → `v0.137.0` (open-telemetry#42760) - `spanmetrics.statusCodeConvention.useOtelPrefix` → `v0.141.0` (open-telemetry#43000) - `spanmetrics.statusCodeConvention.useOtelPrefix` had no `reference_url` in the original registration, so I used the implementing PR as the reference URL as the issue was only partially fixed. - `connector_test.go` already imports `google.golang.org/grpc/metadata`, so the internal `metadata` package was imported with the alias `spanmetricsmetadata` to avoid a name conflict #### Link to tracking issue Part of open-telemetry#46116 #### Testing - `make` in the component directory - `make gogci` in root dir #### Documentation - `connector/spanmetricsconnector/documentation.md` was generated via `make generate`
Description
Adds a feature gate to use the latest semantic conventions for the status code attribute on generated metrics. Changes:
status.code=STATUS_CODE_ERRORotel.status_code=ERRORstatus.code=STATUS_CODE_OKotel.status_code=OKstatus.code=STATUS_CODE_UNSETLink to tracking issue
Partially Fixes #42103
Testing
Added new test case
"Test using new status code attribute"Documentation
README updated, changelog entry added