[chart/opentelemetry-ebpf-instrumentation] only configure prometheus_export when a Service can scrape it - #2360
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25f7a65e1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {{- if not (include "obi.prometheusExportEnabled" .) }} | ||
| {{- $_ := unset $config "prometheus_export" }} |
There was a problem hiding this comment.
When service.enabled is false—as it is by default—but Prometheus uses pod discovery, podAnnotations, or a separately managed Service to scrape the DaemonSet, this unconditionally deletes the configured prometheus_export stanza. The endpoint therefore stops listening after this patch upgrade and these valid scrape setups lose all application metrics; preserve explicitly configured exporters or provide a separate opt-out instead of treating the chart-managed Service as the only possible scraping path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this setting can also be enabled via config without this preset which requires a service
…export when a Service can scrape it Signed-off-by: NimrodAvni78 <nimrod@coralogix.com>
25f7a65 to
5f47eb6
Compare
| {{/* | ||
| Generate the configmap data based on preset and configuration values | ||
| */}} |
There was a problem hiding this comment.
| {{/* | |
| Generate the configmap data based on preset and configuration values | |
| */}} |
| true | ||
| {{- end -}} | ||
| {{- end }} | ||
|
|
There was a problem hiding this comment.
| {{/* | |
| Generate the configmap data based on preset and configuration values | |
| */}} |
|
|
||
| {{- define "obi.configData" -}} | ||
| {{- $config := deepCopy .Values.config.data }} | ||
| {{- if not (include "obi.prometheusExportEnabled" .) }} |
There was a problem hiding this comment.
Do we need an additional template here? Can we simplify with
{{- if not .Values.service.enabled }}
instead?
| - ALL | ||
| {{- end }} | ||
| {{- $promExport := dict }} | ||
| {{- if include "obi.prometheusExportEnabled" . }} |
|
@NimrodAvni78 Pls rebase + see Review |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
marcschaeferger
added
bug
|
@NimrodAvni78 Can you please address the review and rebase |
7 participants
Description
OBI defaulted to enable prometheus exporter.
But the prometheus exporter expires Prometheus metric children only while serving a scrape.
So without anyone scrapping it (no service) then metrics were not evicted and memory consumption grows without eviction
This fixes to only enable prometheus export when service is explicitly turned on
Link to tracking issue
Authorship