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.

Sign up for GitHub

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

Jump to bottom

Add recommendation for well-known k8s labels #349

Closed
wants to merge 1 commit into from
Closed

Add recommendation for well-known k8s labels #349

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions docs/resource/k8s.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,3 +211,10 @@ A CronJob creates Jobs on a repeating schedule.
<!-- endsemconv -->

[DocumentStatus]: https://github.com/open-telemetry/opentelemetry-specification/tree/v1.22.0/specification/document-status.md

## Well-Known Labels

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 •
edited
Loading

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?

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 •
edited
Loading

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?

* The Well-Known Label `app.kubernetes.io/version` SHOULD align to the OpenTelemetry resource attribute `service.version`.
AlexanderWert marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +219 to +220
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
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.

Loading