Align Beyla setting Service Name/Namespace/Instance according to OTEL collector #1415

mariomac · 2024-11-27T15:02:13Z

Addresses issue: grafana/k8s-monitoring-helm#942

Aimed at Beyla 2.0, adds the following breaking changes with regards to Beyla 1.9:

Removes BEYLA_KUBE_META_SOURCE_LABEL_SERVICE_NAME and BEYLA_KUBE_META_SOURCE_LABEL_SERVICE_NAMESPACE environment variables.
Removes attributes > kubernetes > meta_source_labels YAML config section.
Adds BEYLA_KUBE_ANNOTATIONS_SERVICE_NAME, BEYLA_KUBE_ANNOTATIONS_SERVICE_NAMESPACE, BEYLA_KUBE_LABELS_SERVICE_NAME and BEYLA_KUBE_LABELS_SERVICE_NAMESPACE environent variables. They are now a string list.
Adds attributes > kubernetes > meta_naming_sources YAML config section.
Changes the way the Instance ID is composed in K8s applications from kube-pod:container-name to kube-ns.kube-pod.container-name.

Now the way of composing service name/namespace and instance ID are in parity with the OTEL operator, but the configuration is slightly different.

To enable service naming from labels, the OTEL operator relies on a boolean useLabelsForResourceAttributes field in the Instrumenter CRD
The equivalent configuration in Beyla would be setting the following configuraiton option:

attributes:
  kubernetes:
    meta_naming_sources:
      service_name_labels: ["app.kubernetes.io/name"]
      service_namespace_labels: ["app.kubernetes.io/part-of"]

As the Operator and Beyla do not share any configuration structure, I think this difference is not an inconvenience but has the advantage of supporting labels and annotations that might be valid for other contexts outside of OTEL. If at some point Beyla is integrated with the operator (note por any external reader: there are no plans for that), the operator just need to configure Beyla with the above labels when the CRD useLabelsForResourceAttributes field is true.

codecov · 2024-11-27T15:09:35Z

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.94%. Comparing base (043c46e) to head (a56bfcd).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/kubecache/meta/informers_init.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1415      +/-   ##
==========================================
- Coverage   80.97%   80.94%   -0.04%     
==========================================
  Files         146      146              
  Lines       14843    14902      +59     
==========================================
+ Hits        12019    12062      +43     
- Misses       2235     2246      +11     
- Partials      589      594       +5

Flag	Coverage Δ
integration-test	`59.18% <2.43%> (+0.10%)`	⬆️
k8s-integration-test	`60.00% <75.60%> (-0.05%)`	⬇️
oats-test	`33.97% <2.43%> (+0.07%)`	⬆️
unittests	`51.59% <92.68%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rafaelroquetto

LGTM! Nice!

rafaelroquetto · 2024-11-27T15:23:55Z

pkg/beyla/config_test.go

@@ -101,6 +104,9 @@ network:
 	nc.AgentIP = "1.2.3.4"
 	nc.CIDRs = cidr.Definitions{"10.244.0.0/16"}

+	metaSources := kube.DefaultMetadataSources
+	metaSources.ServiceNameLabels = []string{"titi.com/lala"}


You're the best! :)

…e service

grcevski

LGTM! Great work, I think it's critical that we confirm the behaviour of the OTEL env variables and perhaps move the code to reflect that bahaviour.

grcevski · 2024-11-27T16:01:47Z

pkg/internal/kube/store.go

-	if s.sourceLabels.ServiceName != "" {
-		if on, ok := om.Labels[s.sourceLabels.ServiceName]; ok {
-			name = on
+	if nameFromMeta := s.valueFromMetadata(om,


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.

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.

zeitlinger · 2024-11-28T09:27:32Z

I wonder if it would be easier to understand for the user if we add an "operator mode" flag - that hides all the configuration needed?

zeitlinger · 2024-11-28T09:18:48Z

pkg/internal/kube/store.go

+
+var DefaultMetadataSources = MetadataSources{
+	ServiceNameAnnotations: []string{
+		"resource.opentelemetry.io/service.name",


all annotations starting with "resource.opentelemetry.io/" should be supported

if you want a flag (not sure that it's needed) then just "useAnnotationsForResourceAttributes"

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 useAnnotationsForResourceAttributes flag, as it seems a very OTEL-opinionated option and I'd prefer something more flexible to use in other environments and use case.

OK, then maybe make it more generic like #1415 (comment)

zeitlinger · 2024-11-28T09:24:42Z

pkg/internal/kube/store.go

+type MetadataSources struct {
+	ServiceNameAnnotations      []string `yaml:"service_name_annotations" env:"BEYLA_KUBE_ANNOTATIONS_SERVICE_NAME" envSeparator:","`
+	ServiceNamespaceAnnotations []string `yaml:"service_namespace_annotations" env:"BEYLA_KUBE_ANNOTATIONS_SERVICE_NAMESPACE" envSeparator:","`
+	ServiceNameLabels           []string `yaml:"service_name_labels" env:"BEYLA_KUBE_LABELS_SERVICE_NAME" envSeparator:","`


version is missing: https://github.com/open-telemetry/opentelemetry-operator/blob/aa3e781a95def24e52c0dceaf7c33ec5b6193a7e/pkg/constants/env.go#L39

you could make this even more generic:

meta_naming_sources: labels: service.name: ["app.kubernetes.io/name"] service.namespace: ["app.kubernetes.io/part-of"]

Thanks, I like your proposal. Will implement it

zeitlinger · 2024-11-28T09:45:29Z

pkg/internal/kube/store.go

+	); nsFromMeta != "" {
+		namespace = nsFromMeta
+	}
+	return name, namespace


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:

name: https://github.com/open-telemetry/opentelemetry-operator/blob/84200196ff4e0fc9d8999b31891263f093fe7052/pkg/instrumentation/sdk.go#L408-L434

version: https://github.com/open-telemetry/opentelemetry-operator/blob/84200196ff4e0fc9d8999b31891263f093fe7052/pkg/instrumentation/sdk.go#L456-L467

instance.id: https://github.com/open-telemetry/opentelemetry-operator/blob/84200196ff4e0fc9d8999b31891263f093fe7052/pkg/instrumentation/sdk.go#L486-L490

In the Beyla, the fallback values are previously set into the name and namespace variables, then overridden here if valueFromMetadata returns a non-empty value. They were already set as required, so that's why they are not in this PR.

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.

I understand - the important part is to have the same logic - can you point me to the logic?

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).

so it should result in the same outcome - correct?

Yes. My initial concern was not about the outcome but the code reutilization.

mariomac · 2024-11-29T10:42:13Z

Merging. Any further concern or change request can be handled in a new issue/PR.

mariomac added 2 commits November 27, 2024 15:54

Allow setting otel name/ns from annotations too

0628ad4

rename env vars

0ad9414

mariomac added breaking-change 2.0 Needed for Beyla 2.0 labels Nov 27, 2024

rafaelroquetto approved these changes Nov 27, 2024

View reviewed changes

mariomac requested a review from rafaelroquetto November 27, 2024 15:26

Change instance id value

1e34053

mariomac marked this pull request as ready for review November 27, 2024 15:40

mariomac requested review from grcevski and marctc as code owners November 27, 2024 15:40

mariomac requested a review from zeitlinger November 27, 2024 15:40

mariomac added 3 commits November 27, 2024 16:44

rename metasourcelabels to metadatasources

72c6c6d

fix misleading comment

152d49d

Modify integration test to test annotation forwarding from Beyla cach…

2d5be7a

…e service

grcevski approved these changes Nov 27, 2024

View reviewed changes

zeitlinger reviewed Nov 28, 2024

View reviewed changes

mariomac added 2 commits November 28, 2024 11:59

restructure meta source configuration

4aa7929

give preference to OTEL_RESOURCE_ATTRIBUTES

a56bfcd

cyrille-leclerc self-assigned this Nov 29, 2024

mariomac merged commit 77046c4 into grafana:main Nov 29, 2024
13 checks passed

mariomac deleted the otel-name-namespace branch November 29, 2024 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align Beyla setting Service Name/Namespace/Instance according to OTEL collector #1415

Align Beyla setting Service Name/Namespace/Instance according to OTEL collector #1415

mariomac commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

rafaelroquetto left a comment

rafaelroquetto Nov 27, 2024

grcevski left a comment

grcevski Nov 27, 2024

mariomac Nov 28, 2024

zeitlinger commented Nov 28, 2024

zeitlinger Nov 28, 2024

mariomac Nov 28, 2024

zeitlinger Nov 28, 2024

zeitlinger Nov 28, 2024

mariomac Nov 28, 2024

zeitlinger Nov 28, 2024

mariomac Nov 28, 2024 •

edited

Loading

zeitlinger Nov 28, 2024 •

edited

Loading

mariomac Nov 28, 2024

zeitlinger Nov 28, 2024

mariomac Nov 28, 2024

mariomac commented Nov 29, 2024

Align Beyla setting Service Name/Namespace/Instance according to OTEL collector #1415

Align Beyla setting Service Name/Namespace/Instance according to OTEL collector #1415

Conversation

mariomac commented Nov 27, 2024 • edited Loading

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

rafaelroquetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grcevski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeitlinger commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariomac Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

zeitlinger Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariomac commented Nov 29, 2024

mariomac commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

mariomac Nov 28, 2024 •

edited

Loading

zeitlinger Nov 28, 2024 •

edited

Loading