Skip to content

feat(logging-apps): add charts for opentelemetry-collector#1426

Open
cfi2017 wants to merge 4 commits intomainfrom
feat/logging-apps/otelcol-k8s
Open

feat(logging-apps): add charts for opentelemetry-collector#1426
cfi2017 wants to merge 4 commits intomainfrom
feat/logging-apps/otelcol-k8s

Conversation

@cfi2017
Copy link
Copy Markdown
Contributor

@cfi2017 cfi2017 commented Jul 22, 2025

Description

Introduces two references to opentelemetry-collector for use for log and event scraping

Issues

#1425

Checklist

  • This PR contains a description of the changes I'm making
  • I updated the version in Chart.yaml
  • I updated the changelog with an artifacthub.io/changes annotation in Chart.yaml, check the example in the documentation.
  • I updated applicable README.md files using pre-commit run
  • I documented any high-level concepts I'm introducing in docs/
  • CI is currently green and this is ready for review
  • I am ready to test changes after they are applied and released

Signed-off-by: Carlo Field <carlo.field@adfinis.com>
@cfi2017 cfi2017 self-assigned this Jul 22, 2025
@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 22, 2025
cfi2017 added 3 commits July 22, 2025 09:36
Signed-off-by: Carlo Field <carlo.field@adfinis.com>
Signed-off-by: Carlo Field <carlo.field@adfinis.com>
Signed-off-by: Carlo Field <carlo.field@adfinis.com>
@cfi2017 cfi2017 marked this pull request as ready for review July 22, 2025 07:41
@cfi2017 cfi2017 requested a review from a team as a code owner July 22, 2025 07:41
@cfi2017 cfi2017 requested review from Nerkho and inisitijitty July 22, 2025 07:41
@hairmare
Copy link
Copy Markdown
Contributor

hairmare commented Jul 22, 2025

otel-col is already in tracing-apps

if you need it twice, you can deploy tracing-apps twice

the usualy reference architecture lets one instance handle a full cluster tho

@cfi2017
Copy link
Copy Markdown
Contributor Author

cfi2017 commented Jul 22, 2025

otel-col is already in tracing-apps

Since we want otel-col for different use-cases (in this case for instance for log/event collection), does it make sense to keep it in tracing-apps? This PR would be here specifically to act as a replacement for promtail instances, which are currently in logging-apps.

if you need it twice, you can deploy tracing-apps twice

My gut feeling is I don't really like that approach, as it duplicates our installed app-of-apps charts in a rather nontransparent way. Especially since if we install one, we usually also need to install the other - I don't see a big use-case for just having logs or events, but not both.

the usualy reference architecture lets one instance handle a full cluster tho

According to docs, we'd need to then configure some sort of event deduplication. Log collection needs a daemonset as it fetches the logs from the node, but then you'd have multiple instances scraping the Kubernetes API for events.
https://opentelemetry.io/docs/platforms/kubernetes/collector/components/#kubernetes-objects-receiver

@hairmare
Copy link
Copy Markdown
Contributor

Currently the *-apps only have two responsibilities:

  • Tracking chart sources (where we get our charts from, repo URLs and chart names)
  • Tracking chart versions

This would expand considerably on that since the charts would now suddenly contain opinions on much more.

I feel like this is a considerable change that we would want to weight carefully (and also document properly in docs/ if we end up deciding to do it).

duplicates our installed app-of-apps charts in a rather nontransparent way

Does it really tho? It feels like this is a purely cosmetic concern?

@cfi2017
Copy link
Copy Markdown
Contributor Author

cfi2017 commented Jul 23, 2025

Does it really tho? It feels like this is a purely cosmetic concern?

From a user perspective, we currently usually create one logging-apps (or tracing-apps, w/e) manifest with whatever features we want to enable:

manifests/
|- logging-apps.yaml

With the otelcol changes with just one chart, we'd end up with three files semantically:

manifests/
|- logging-apps.yaml
|- logging-apps-otelcol-logs.yaml
|- logging-apps-otelcol-events.yaml

The naming can be substituted but the principle remains. We could also only have two apps, something like this:

manifests/
|- logging-apps.yaml
|- logging-apps-extra-otelcol.yaml

But that also looks weird to me.

It would also cause argocd to show multiple logging-apps (renamed, of course) app-of-apps charts.

You're right, it's mostly a cosmetic change.
Trying to reason the change, the chart we are given is a bit of a polyglot (I'm probably not using that word right) in the sense that it supports multiple use-cases and deployment modes. If this were an edge case I'd absolutely agree with you, but I get the sense that we would only rarely just deploy one instance of these charts.


Throwing another idea out there, seeing as we have the infrastructure to maintain our own charts, we could do something like this:

logging-apps
|- adfinis-otelcol-chart
   |- otelcol-k8s-events (just a dependency)
   |- otelcol-k8s-logs (just a dependency)

With this we could also set sane default values like setting the otelcol image flavour to k8s and the deployment modes for each chart to default to DaemonSet / Deployment respectively.

The downside of this is having to maintain yet another helm chart.


And finally, we could put some more time into figuring out how hard it'd actually be to do event deduplication - I haven't looked into this a lot since it seems like it'll come with a rats tail of other constraints, but perhaps there is a documented path for this? If we can deduplicate events we could just go the DaemonSet route and entirely do away with the Deployment.

@eyenx
Copy link
Copy Markdown
Member

eyenx commented Sep 1, 2025

Throwing another idea out there, seeing as we have the infrastructure to maintain our own charts, we could do something like this:

logging-apps
|- adfinis-otelcol-chart
   |- otelcol-k8s-events (just a dependency)
   |- otelcol-k8s-logs (just a dependency)

I like this approach. Although we would maintain yet another chart, we won't need to update otelcol in three different spots.

Now the question remains, we would put it in logging-apps or tracing-apps? or deprecate those two and start a new obervability-apps or even move it to infra-apps?

@hairmare
Copy link
Copy Markdown
Contributor

hairmare commented Sep 1, 2025

how about using an ApplicationSet instead of the version from logging-apps?

@cfi2017
Copy link
Copy Markdown
Contributor Author

cfi2017 commented Sep 18, 2025

how about using an ApplicationSet instead of the version from logging-apps?

Do we use ApplicationSets elsewhere at the moment (excluding potential customer workloads)?

What do you think about the idea of maintaining the combination as a helm chart as suggested in the comment above yours?

@hairmare
Copy link
Copy Markdown
Contributor

hairmare commented Sep 18, 2025

What do you think about the idea of maintaining the combination as a helm chart as suggested in the comment above yours?

It what prompted me to suggest an ApplicationSet because that might already cover a lot of bases.

It's pretty clear that we would need a solution that support an n amount of otel-collectors, so the chart would probably need to do some magick to support something aloong the lines of otelCollectors: [] in values.

We could also investigate if there is any potential to use an operator to manage the otel-col instances, maybe by leveraging https://github.com/open-telemetry/opentelemetry-operator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants