Replacing semconv package #1784
Conversation
…lemetry.io/otel/semconv in opampextension
…lemetry.io/otel/semconv in k8sprocessor
…lemetry.io/otel/semconv in sumologicschemaprocessor
|
@pankaj101A I don't have access to the google doc you posted, and I don't see anything about the semconv package in the release notes for 0.129.0. Can you please link some supporting documentation? |
updated description, access provided for sheet |
|
@echlebek open-telemetry/opentelemetry-collector#13071 has deprecation details |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the collector’s semantic conventions package with the OTEL semconv module and bumps OTEL dependencies to v1.37.0, updating all imports and references accordingly.
- Swapped imports from
go.opentelemetry.io/collector/semconvtogo.opentelemetry.io/otel/semconvand adjusted attribute key/value usage - Updated all code, tests, and
go.modfiles to usestring(conventions.*Key)(or.Value.AsString()) instead of oldAttribute*constants - Bumped
go.opentelemetry.io/oteland related modules from v1.35.0 to v1.37.0
Reviewed Changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/processor/sumologicschemaprocessor/go.mod | Bumped OTEL version, removed collector semconv requirement |
| pkg/processor/sumologicschemaprocessor/cloud_namespace_processor.go | Switched semconv import and replaced Attribute* with new key usage |
| pkg/processor/k8sprocessor/processor_test.go | Updated tests to use new key-based conventions |
| pkg/processor/k8sprocessor/pod_association.go | Replaced old attribute constants with string(conventions.*Key) |
| pkg/processor/k8sprocessor/options_test.go | Updated test fields list to use new key constants |
| pkg/processor/k8sprocessor/options.go | Swapped semconv import and adjusted switch cases |
| pkg/processor/k8sprocessor/kube/kube.go | Updated tag defaults to use new key constants |
| pkg/processor/k8sprocessor/go.mod | Bumped OTEL version, removed collector semconv requirement |
| pkg/extension/opampextension/opamp_agent_test.go | Updated test imports and attribute puts to use new key constants |
| pkg/extension/opampextension/opamp_agent.go | Swapped semconv import and attribute gets to use new key constants |
| pkg/extension/opampextension/go.mod | Bumped OTEL version, removed collector semconv requirement |
Comments suppressed due to low confidence (2)
pkg/processor/sumologicschemaprocessor/cloud_namespace_processor.go:76
- Consider adding unit tests for addCloudNamespaceAttribute to verify that the processor correctly adds
cloud.namespacefor AWS ECS and AWS Elastic Beanstalk using the new OTEL semconv package.
func addCloudNamespaceAttribute(attributes pcommon.Map) {
pkg/processor/sumologicschemaprocessor/cloud_namespace_processor.go:22
- [nitpick] The import alias
conventionsis used here but some modules import the same package assemconv; consider standardizing the alias (e.g., always usesemconv) for consistency across the codebase.
conventions "go.opentelemetry.io/otel/semconv/v1.18.0"
| case metadataContainerID, string(conventions.ContainerIDKey): | ||
| p.rules.ContainerID = true | ||
| case metadataContainerImage, conventions.AttributeContainerImageName: | ||
| case metadataContainerImage, string(conventions.ContainerImageNameKey): | ||
| p.rules.ContainerImage = true | ||
| case metadataContainerName, conventions.AttributeContainerName: | ||
| case metadataContainerName, string(conventions.ContainerNameKey): | ||
| p.rules.ContainerName = true | ||
| case metadataCronJobName, conventions.AttributeK8SCronJobName: | ||
| case metadataCronJobName, string(conventions.K8SCronJobNameKey): | ||
| p.rules.CronJobName = true | ||
| case metadataDaemonSetName, conventions.AttributeK8SDaemonSetName: | ||
| case metadataDaemonSetName, string(conventions.K8SDaemonSetNameKey): | ||
| p.rules.DaemonSetName = true | ||
| case metadataDeploymentName, conventions.AttributeK8SDeploymentName: | ||
| case metadataDeploymentName, string(conventions.K8SDeploymentNameKey): | ||
| p.rules.DeploymentName = true | ||
| case metadataHostName, conventions.AttributeHostName: | ||
| case metadataHostName, string(conventions.HostNameKey): | ||
| p.rules.HostName = true | ||
| case metadataJobName, conventions.AttributeK8SJobName: | ||
| case metadataJobName, string(conventions.K8SJobNameKey): | ||
| p.rules.JobName = true | ||
| case metadataNamespace, conventions.AttributeK8SNamespaceName: | ||
| case metadataNamespace, string(conventions.K8SNamespaceNameKey): | ||
| p.rules.Namespace = true | ||
| case metadataNodeName, conventions.AttributeK8SNodeName: | ||
| case metadataNodeName, string(conventions.K8SNodeNameKey): | ||
| p.rules.NodeName = true | ||
| case metadataPodID, conventions.AttributeK8SPodUID: | ||
| case metadataPodID, string(conventions.K8SPodUIDKey): | ||
| p.rules.PodUID = true | ||
| case metadataPodName, conventions.AttributeK8SPodName: | ||
| case metadataPodName, string(conventions.K8SPodNameKey): | ||
| p.rules.PodName = true | ||
| case metadataReplicaSetName, conventions.AttributeK8SReplicaSetName: | ||
| case metadataReplicaSetName, string(conventions.K8SReplicaSetNameKey): | ||
| p.rules.ReplicaSetName = true | ||
| case metadataServiceName, metadataOtelSemconvServiceName: | ||
| p.rules.ServiceName = true | ||
| case metadataStartTime, metadataOtelPodStartTime: | ||
| p.rules.StartTime = true | ||
| case metadataStatefulSetName, conventions.AttributeK8SStatefulSetName: | ||
| case metadataStatefulSetName, string(conventions.K8SStatefulSetNameKey): | ||
| p.rules.StatefulSetName = true | ||
| case deprecatedMetadataClusterName, conventions.AttributeK8SClusterName: | ||
| case deprecatedMetadataClusterName, string(conventions.K8SClusterNameKey): |
There was a problem hiding this comment.
[nitpick] There's repeated use of string(conventions.<Key>) throughout this switch; consider defining local constants or a small helper function to convert keys to string and reduce duplication.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Better to use string based casting itself here, even otel collector core repo uses the same.
There was a problem hiding this comment.
Why is the conversion necessary? attribute.Key is type string.
There was a problem hiding this comment.
The function could take ...attribute.Key instead of ...string and it would clean the code up a bit.
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-go/blob/bd26298747209abeba2d415e8392164a9bc88120/semconv/v1.4.0/resource.go#L440
Yes, resource attributes seems to be string type, can we can remove this explicit typecast @pankaj101A
There was a problem hiding this comment.
Why is the conversion necessary?
attribute.Keyis type string.
type is Key which is alias to type string, in go i need to explicitly convert a alias type to string even if it stores string value
There was a problem hiding this comment.
https://github.com/open-telemetry/opentelemetry-go/blob/bd26298747209abeba2d415e8392164a9bc88120/semconv/v1.4.0/resource.go#L440 Yes, resource attributes seems to be string type, can we can remove this explicit typecast @pankaj101A
There was a problem hiding this comment.
The function could take
...attribute.Keyinstead of...stringand it would clean the code up a bit.
values are config which are provided by user in config files, i guess those are passed on collector as string only
Replacing http://go.opentelemetry.io/collector/semconv with http://go.opentelemetry.io/otel/semconv
Breaking change in 0.129
https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.129.0
https://docs.google.com/spreadsheets/d/1-m3kpHnw7P80T7KTUDJA_lTBG7wy_EXTo384gmO9UeY/edit?gid=1677611055#gid=1677611055