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

Add recommendation for well-known k8s labels #349

Closed

Conversation

smithclay
Copy link

@smithclay smithclay commented Sep 26, 2023

Changes

Adds recommendations for mapping Kubernetes well-known labels to existing OpenTelemetry resource attributes.

See #236 for original issue.

Merge requirement checklist

@smithclay smithclay requested review from a team September 26, 2023 21:36
Comment on lines +219 to +220
* The Well-Known Label `app.kubernetes.io/name` SHOULD align to the OpenTelemetry resource attribute `service.name`.
* The Well-Known Label `app.kubernetes.io/version` SHOULD align to the OpenTelemetry resource attribute `service.version`.
Copy link
Member

Choose a reason for hiding this comment

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

In #236 you link to a potential collector config that would implement this but if we already standardize a mapping here, I think the k8sattributesprocessor in the collector should be adapted to provide this mapping out of the box (potentially with a simple opt-in flag).
Did you discuss this addition with the collector folks owning the processor? I think it would make sense to align this with them.

Copy link
Author

Choose a reason for hiding this comment

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

Seems like a nice thing to offer to get this more easily adopted, opened an issue @ open-telemetry/opentelemetry-collector-contrib#27261

Copy link
Author

Choose a reason for hiding this comment

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

For now, think this also makes sense in the default helm charts. Issue tracked here: open-telemetry/opentelemetry-helm-charts#905

Comment on lines +219 to +220
* The Well-Known Label `app.kubernetes.io/name` SHOULD align to the OpenTelemetry resource attribute `service.name`.
* The Well-Known Label `app.kubernetes.io/version` SHOULD align to the OpenTelemetry resource attribute `service.version`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a technical nit, but more about organization of semantic conventions in general.

In this PR we K8s resource conventions requiring a certain value for service.name, in #312 we have an attempt to extend the the service.instance.id definition with K8s specific data.

It would be great to have a consistent way to specify such mappings. I'm slightly in favor of the approach this PR takes (defining values for general attributes in the domain specific conventions).

Copy link
Contributor

Choose a reason for hiding this comment

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

Elaborating a bit on this point, after chatting with @jpkrohling:

Given that both #312 and this PR are merged: if I'd want to instrument a K8s service, the conventions would be spread out across places:

  • setting service.name is specified in resource/k8s.md
  • setting service.version is specified in resource/k8s.md
  • setting service.instance.id is specified in an algorithm in resource/README.md

I think we should think about consistency before merging any of those PRs. Like:

  • Should we consistently define algorithms for service.* attributes, like proposed in Define a common algorithm for service.instance.id #312 for service.instance.id?
  • Should we have consistently have area-specific overrides for service.* attributes, like what's proposed in this PR for service.name?
  • Or are we fine with having a mix?

We needn't come to a final conclusion before merging anything experimental, but we'd need to before declaring anything stable.


There are certain Kubernetes [Well-Known Labels and Annotations](https://kubernetes.io/docs/reference/labels-annotations-taints/) that SHOULD align to OpenTelemetry resource attributes. Labels are typically converted to existing OpenTelemetry attributes automatically using an OpenTelemetry Collector component like the [Kubernetes Attribute Processor](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/k8sattributesprocessor).

* The Well-Known Label `app.kubernetes.io/name` SHOULD align to the OpenTelemetry resource attribute `service.name`.
Copy link

@jinja2 jinja2 Oct 2, 2023

Choose a reason for hiding this comment

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

This label is usually used to express the generic application name, whereas app.kubernetes.io/instance is the recommended label key to indicate a specific installation of an application in k8s. The service.name attr is defined as the logical name of a service. I assume this is a unique name differentiating a service installation from another; could actually do with more clarification on this.

Let's say a k8s environment has multiple installations of the application kafka, named kafka-a, kafka-b and kafka-c deployed using a kafka helm chart (standard label mapping for helm follows the same recommendation). In this case, all pods from the 3 different statefulset are likely to have app.kubernetes.io/name = kafka and the value of label app.kubernetes.io/instance will be the specific installation/usecase of the app kafka. Depending on internal conventions, users might want service.name to reflect the specific instance name instead of just kafka in this case. All this to say, there is some ambiguity here about which label maps to service.name and imo the convention should just suggest the possible well-known labels k8s users can consider.

Copy link
Author

Choose a reason for hiding this comment

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

From a language perspective: how about replacing SHOULD with RECOMMENDED + add the note around ambiguity around app.kubernetes.io/instance?


There are certain Kubernetes [Well-Known Labels and Annotations](https://kubernetes.io/docs/reference/labels-annotations-taints/) that SHOULD align to OpenTelemetry resource attributes. Labels are typically converted to existing OpenTelemetry attributes automatically using an OpenTelemetry Collector component like the [Kubernetes Attribute Processor](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/k8sattributesprocessor).

* The Well-Known Label `app.kubernetes.io/name` SHOULD align to the OpenTelemetry resource attribute `service.name`.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a convention about service.name which is declared stable:

MUST be the same for all instances of horizontally scaled services. If the value was not specified, SDKs MUST fallback to unknown_service: concatenated with process.executable.name, e.g. unknown_service:bash. If process.executable.name is not available, the value MUST be set to unknown_service.

I'm not sure whether we can "alter" or "amend" a stable attribute definition like proposed here.

Copy link
Author

@smithclay smithclay Oct 24, 2023

Choose a reason for hiding this comment

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

Perhaps we can say this is allowed as it doesn't modify or amend the convention itself, but is related to something something external (k8s labels) and is an implementation (in k8s) recommendation?

Alternately, could this be organized differently or put somewhere else?

Copy link

github-actions bot commented Feb 3, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 3, 2024
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants