Skip to content

[FEATURE] adding annotation discovery preset #1724

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

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

Conversation

nicolastakashi
Copy link
Contributor

This pull request introduces support for annotation-based discovery of logs and metrics in the OpenTelemetry Collector Helm chart. It adds new configuration options, updates templates to handle the new presets, and ensures mutual exclusivity between certain features. Below is a summary of the most important changes:

New Feature: Annotation-Based Discovery

  • Added a new annotationDiscovery preset in values.yaml that allows enabling discovery of logs and metrics based on Kubernetes annotations. (charts/opentelemetry-collector/values.yaml, charts/opentelemetry-collector/values.yamlR80-R85)
  • Introduced new Helm template definitions for annotationDiscoveryConfig and applyAnnotationDiscoveryConfig to configure the OpenTelemetry Collector for annotation-based discovery. These include setting up pipelines and receivers for logs and metrics. (charts/opentelemetry-collector/templates/_config.tpl, charts/opentelemetry-collector/templates/_config.tplR269-R308)

Configuration Updates

  • Updated _config.tpl to include applyAnnotationDiscoveryConfig when either logs or metrics discovery is enabled. (charts/opentelemetry-collector/templates/_config.tpl, [1] [2]
  • Added a warning in NOTES.txt to ensure logsCollection and annotationDiscovery.logs are not enabled simultaneously, as they are mutually exclusive. (charts/opentelemetry-collector/templates/NOTES.txt, charts/opentelemetry-collector/templates/NOTES.txtR66-R69)

Kubernetes Role Adjustments

Pod Configuration

  • Modified _pod.tpl to set the K8S_NODE_NAME environment variable when annotation-based discovery is enabled and to adjust volume mounts for logs collection. (charts/opentelemetry-collector/templates/_pod.tpl, [1] [2] [3]

@nicolastakashi
Copy link
Contributor Author

Closes #1634

@nicolastakashi nicolastakashi marked this pull request as ready for review June 27, 2025 17:49
@nicolastakashi
Copy link
Contributor Author

cc @ChrsMark

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Looks good overall with few comments.
Also should we add some content about this at https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/README.md?

Comment on lines +67 to +69
{{- if and .Values.presets.logsCollection.enabled .Values.presets.annotationDiscovery.logs.enabled }}
{{- fail "[ERROR] logsCollection and annotationDiscovery.logs are mutually exclusive" }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should they be exclusive? Could we somehow by default collect both ways, as annotation one allows you to do custom parsing via file log, while logsCollection would be default and could somehow ignore the annotation based ones?

Maybe for the future though

Copy link
Member

@ChrsMark ChrsMark Jul 2, 2025

Choose a reason for hiding this comment

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

If both logsCollection and annotationDiscovery.logs are enabled logs of annotated Pods will be collected twice. This is log duplication that we have to avoid.

We don't have a way to configure logsCollection to skip annotated Pods :).

To collect all logs without extra parsing the following can be used:

      default_annotations:
        io.opentelemetry.discovery.logs/enabled: "true"

This will have the Collector to Collect all logs by default without any parsing applied as logsCollection would do. Then annotated Pods will be handled according to the provided annotations.

Is there any use-case this approach wouldn't cover?

discovery:
enabled: true
default_annotations:
io.opentelemetry.discovery.logs/enabled: "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this configureable?

I think it useful to not collect all by default 🤔

Copy link
Member

@ChrsMark ChrsMark Jul 2, 2025

Choose a reason for hiding this comment

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

We could make this configurable but as explained at #1724 (comment) the reason for this is to act as drop-in replacement of logsCollection which collects all logs anyways.

@nicolastakashi nicolastakashi requested a review from ChrsMark July 4, 2025 11:13
@nicolastakashi nicolastakashi requested a review from povilasv July 4, 2025 11:13
Signed-off-by: Nicolas Takashi <[email protected]>
Signed-off-by: Nicolas Takashi <[email protected]>
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM, thank's!

@@ -77,6 +77,12 @@ presets:
clusterMetrics:
enabled: false

annotationDiscovery:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a new test in the ci folder for this preset?

Comment on lines +107 to +108
headers:
[]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
headers:
[]
headers: []

Comment on lines +393 to +394
hostAliases:
[]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
hostAliases:
[]
hostAliases: []

Comment on lines +504 to +505
startupProbe:
{}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
startupProbe:
{}
startupProbe: {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants