Align Beyla setting Service Name/Namespace/Instance according to OTEL collector #1415
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,10 +36,29 @@ func qName(om *informer.ObjectMeta) qualifiedName { | |
| return qualifiedName{name: om.Name, namespace: om.Namespace, kind: om.Kind} | ||
| } | ||
|
|
||
| // MetadataSources allow overriding some metadata from kubernetes labels and annotations | ||
| type MetadataSources struct { | ||
| ServiceNameAnnotations []string `yaml:"service_name_annotations" env:"BEYLA_KUBE_ANNOTATION_SERVICE_NAME" envSeparator:","` | ||
| ServiceNamespaceAnnotations []string `yaml:"service_namespace_annotations" env:"BEYLA_KUBE_ANNOTATION_SERVICE_NAMESPACE" envSeparator:","` | ||
| ServiceNameLabels []string `yaml:"service_name_labels" env:"BEYLA_KUBE_LABEL_SERVICE_NAME" envSeparator:","` | ||
| ServiceNamespaceLabels []string `yaml:"service_namespace_labels" env:"BEYLA_KUBE_LABEL_SERVICE_NAMESPACE" envSeparator:","` | ||
| } | ||
|
|
||
| var DefaultMetadataSources = MetadataSources{ | ||
| ServiceNameAnnotations: []string{ | ||
| "resource.opentelemetry.io/service.name", | ||
|
There was a problem hiding this comment. all annotations starting with "resource.opentelemetry.io/" should be supported if you want a flag (not sure that it's needed) then just "useAnnotationsForResourceAttributes" There was a problem hiding this comment. Yeah, this PR currently changes what we already support: service name, namespace and instance ID. Service version and deployment environment will go into another PR, as they are attributes that aren't currently provided by Beyla. I'd prefer not using the There was a problem hiding this comment. OK, then maybe make it more generic like #1415 (comment) |
||
| }, | ||
| ServiceNamespaceAnnotations: []string{ | ||
| "resource.opentelemetry.io/service.namespace", | ||
| }, | ||
| // default by empty. If the OTEL operator Instrumentation CRD sets useLabelsForResourceAttributes: true, | ||
| // the values below should be populated so: | ||
| // - `app.kubernetes.io/name` becomes `service.name` | ||
| // - `app.kubernetes.io/version` becomes `service.version` | ||
| // - `app.kubernetes.io/part-of` becomes `service.namespace` | ||
| // - `app.kubernetes.io/instance` becomes `service.instance.id` | ||
| ServiceNameLabels: nil, | ||
| ServiceNamespaceLabels: nil, | ||
| } | ||
|
|
||
| // Store aggregates Kubernetes information from multiple sources: | ||
|
|
@@ -79,10 +98,10 @@ type Store struct { | |
| // they receive is already present in the store | ||
| meta.BaseNotifier | ||
|
|
||
| metadataSources MetadataSources | ||
| } | ||
|
|
||
| func NewStore(kubeMetadata meta.Notifier, metadataSources MetadataSources) *Store { | ||
| log := dblog() | ||
| db := &Store{ | ||
| log: log, | ||
|
|
@@ -96,7 +115,7 @@ func NewStore(kubeMetadata meta.Notifier, sourceLabels MetaSourceLabels) *Store | |
| otelServiceInfoByIP: map[string]OTelServiceNamePair{}, | ||
| metadataNotifier: kubeMetadata, | ||
| BaseNotifier: meta.NewBaseNotifier(log), | ||
| metadataSources: metadataSources, | ||
| } | ||
| kubeMetadata.Subscribe(db) | ||
| return db | ||
|
|
@@ -289,17 +308,36 @@ func (s *Store) serviceNameNamespaceForMetadata(om *informer.ObjectMeta) (string | |
| } else { | ||
| name, namespace = s.serviceNameNamespaceForOwner(om) | ||
| } | ||
| if nameFromMeta := s.valueFromMetadata(om, | ||
|
There was a problem hiding this comment. Doing this here will override any service names set by the OTEL environment variables. I think this will introduce a difference in how it will be handled by the SDK, because environment variables passed to the process directly will, "I think", override whatever else is set. I think it might be better if we added this override in here: func (s *Store) serviceNameNamespaceOwnerID(ownerKey, name, namespace string) (string, string) {
serviceName := name
serviceNamespace := namespace
...
}Right after we set them from whatever k8s found, we can override them based on the labels and annotations and then let the envvar code do its thing. But we should confirm with the SDKs team if my understanding is correct that the env variables take precedence. There was a problem hiding this comment. Good catch! I reordered the preferences and modified some unit and integration tests to make sure that the environment variables take precedence over annotations or labels. |
||
| s.metadataSources.ServiceNameAnnotations, | ||
| s.metadataSources.ServiceNameLabels, | ||
| ); nameFromMeta != "" { | ||
| name = nameFromMeta | ||
| } | ||
| if nsFromMeta := s.valueFromMetadata(om, | ||
| s.metadataSources.ServiceNamespaceAnnotations, | ||
| s.metadataSources.ServiceNamespaceLabels, | ||
| ); nsFromMeta != "" { | ||
| namespace = nsFromMeta | ||
| } | ||
| return name, namespace | ||
|
There was a problem hiding this comment. I think the fallback values are missing (at least not in this PR) - which are maybe the most important part. In the operator, those are:
There was a problem hiding this comment. In the Beyla, the fallback values are previously set into the That's the reason I was mentioning that moving this code into a shared library could be complex due to the way multiple components could populate/store the same information. There was a problem hiding this comment. I understand - the important part is to have the same logic - can you point me to the logic? There was a problem hiding this comment. On one side, Beyla does not store the metadata from the Pod owners (DaemonSet, Deployment, Job, etc...) so we cannot query the fallbacks one by one. Instead, Beyla infers the pod owner name from the actual pod name (assuming the kubernetes pod naming conventions). So at the point we execute the above, service.name already contains the value of of one of the fallbacks (which at the end are nothing that the Pod owner name). Is then that, if the service name/namespace can't be inferred from env/annotations/labels (in that priority), the original value is kept (the fallback). There was a problem hiding this comment. so it should result in the same outcome - correct? There was a problem hiding this comment. Yes. My initial concern was not about the outcome but the code reutilization. |
||
| } | ||
|
|
||
| // function implemented to provide consistent service metadata naming across multiple | ||
| // OTEL implementations: OTEL operator, Loki and Beyla | ||
| // https://github.com/grafana/k8s-monitoring-helm/issues/942 | ||
| func (s *Store) valueFromMetadata(om *informer.ObjectMeta, annotationNames, labelNames []string) string { | ||
| for _, key := range annotationNames { | ||
| if val, ok := om.Annotations[key]; ok { | ||
| return val | ||
| } | ||
| } | ||
| for _, key := range labelNames { | ||
| if val, ok := om.Labels[key]; ok { | ||
| return val | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| // ServiceNameNamespaceForIP returns the service name and namespace for a given IP address | ||
|
|
||
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.
You're the best! :)