Skip to content
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

Resource Attributes - refactor and add missing attributes #2111

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

edeNFed
Copy link
Contributor

@edeNFed edeNFed commented Jan 1, 2025

This PR do the following changes:

  • Unify all resource attributes logic to one package
  • Remove anything related to device id and communication with kubelet from OpAMP / data-collector
  • Modify eBPF director, Pods Webhook and OpAMP server to use the new unified logic.
  • Use k8sattributeprocessor in odigos-data-collection to fill the rest of the needed kubernetes resource attributes

@edeNFed edeNFed force-pushed the add-missing-kube-resource-attributes branch from 0b04c97 to 32bcd16 Compare January 2, 2025 07:09
common/resourceattributes/root.go Show resolved Hide resolved
instrumentation/factory.go Outdated Show resolved Hide resolved
instrumentor/controllers/instrumentationdevice/manager.go Outdated Show resolved Hide resolved
@edeNFed edeNFed force-pushed the add-missing-kube-resource-attributes branch from 86428fa to c1f6c3d Compare January 4, 2025 07:26
@BenElferink BenElferink added bug Something isn't working enhancement New feature or request labels Jan 6, 2025
@edeNFed edeNFed force-pushed the add-missing-kube-resource-attributes branch from 7ab9e5d to 511aad1 Compare January 10, 2025 07:44
@edeNFed edeNFed force-pushed the add-missing-kube-resource-attributes branch from 6183e8a to 9396435 Compare January 10, 2025 15:49
ResourceAttributes: utils.GetResourceAttributes(kd.pw, kd.pod.Name),
InitialConfig: sdkConfig,
ServiceName: OtelServiceName,
ResourceAttributes: resourceattributes.AfterPodStart(&resourceattributes.ContainerIdentifier{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change will drop the deployment.name attribute we used to have - now the processor will add them. Why not add more attributes here if we can get them, instead of relying on the processor?

Comment on lines +171 to +184
"extract": config.GenericMap{
"metadata": []string{
"k8s.deployment.name",
"k8s.statefulset.name",
"k8s.daemonset.name",
"k8s.cronjob.name",
"k8s.job.name",
"k8s.cluster.uid",
"k8s.node.name",
"container.id",
"container.image.name",
"container.image.tag",
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make the add cluster info action obsolete?

semconv "go.opentelemetry.io/otel/semconv/v1.26.0"
)

type Attributes []attribute.KeyValue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type definition introduces a breaking change to the factory API for eBPF instrumentation.
It adds some better syntax, but I prefer to just use the Otel type. We could still have the util functions here.

Comment on lines +10 to +20
Upsert modificationFunc = func(origVal string, newVal string) string {
return newVal
}

AppendWithSpace modificationFunc = func(origVal string, newVal string) string {
return origVal + " " + newVal
}

AppendWithComma modificationFunc = func(origVal string, newVal string) string {
return origVal + "," + newVal
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If those are just used for testing, better to move them to the _test file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants