Skip to content

[amazon-cloudwatch-observability] refactor: single-switch SM/PM scraping under otelContainerInsights.prometheusScrape - #332

Open
wenegiemepraise wants to merge 26 commits into
aws-observability:mainfrom
wenegiemepraise:helm-flag-restructure
Open

[amazon-cloudwatch-observability] refactor: single-switch SM/PM scraping under otelContainerInsights.prometheusScrape#332
wenegiemepraise wants to merge 26 commits into
aws-observability:mainfrom
wenegiemepraise:helm-flag-restructure

Conversation

@wenegiemepraise

Copy link
Copy Markdown

Summary

Collapse the SM/PM scraping enablement into a single switch. Previously, turning on the
feature spanned several interacting flags (prometheusCRDs.install,
otelContainerInsights.serviceMonitor/podMonitor.enabled, deep
agent.prometheus.targetAllocator.*). This consolidates them under one nested bundle:

otelContainerInsights:
  enabled: true                 # the one switch: CRDs + Target Allocator + prometheusCR + RBAC
  prometheusScrape:
    enabled: true               # single opt-out: false => OTEL CI without SM/PM scraping
    allocationStrategy: per-node
    crds:
      install: auto             # auto|always|never
    serviceMonitor: { enabled: true }
    podMonitor:     { enabled: true }

otelContainerInsights.enabled: true now makes SM/PM scraping work end to end; a single
prometheusScrape.enabled: false opts out while keeping the rest of OTEL Container Insights.

Notes for reviewers

  • API/UX change. These keys never shipped in a released chart version, so there's no
    external contract to preserve — the old prometheusCRDs / otelContainerInsights. serviceMonitor|podMonitor top-level keys are moved, not aliased.
  • Gating helpers in _helpers.tpl (otelCIScrapeEnabled, prometheusCRDsEnabled) now read
    from the prometheusScrape bundle.

Dependencies

Top of the helm stack — refactors keys introduced by #329 (v2 OTLP path), #330
(per-node) and #331 (CRD bundling). Merge those first; this PR's diff includes them until
they land.

Testing

helm template:

  • otelContainerInsights.enabled=true → CRDs render (2), prometheuscr path renders,
    allocationStrategy: "per-node".
  • + prometheusScrape.enabled=false → prometheuscr and bundled CRDs both drop to 0
    (scraping off, rest of CI intact).

helm lint clean (pre-existing fluent-bit-windows region warning is unrelated).

…path

Route Prometheus ServiceMonitor/PodMonitor metrics through the
otelContainerInsights v2 pipeline to CloudWatch using the Target Allocator's
prometheusCR discovery:

- _otel-container-insights-config.tpl: add the prometheus/cw_k8s_ci_v0_prometheuscr
  receiver (Target Allocator endpoint, mTLS) and the metrics/cw_k8s_ci_v0_prometheuscr
  pipeline (filter/scrape_metadata, metricstarttime, set_cluster_name,
  set_scope_prometheuscr, resourcedetection, batch) exported via otlphttp.
- _helpers.tpl: add the cloudwatch-agent.otelCIScrapeEnabled helper
  (otelContainerInsights.enabled AND targetAgent AND serviceMonitor/podMonitor on).
- cloudwatch-agent-custom-resource.yaml: render targetAllocator.enabled and
  prometheusCR.enabled on the AmazonCloudWatchAgent CR when otelCIScrape is on.
- target-allocator-clusterrole.yaml / -clusterrolebinding.yaml: grant the TA RBAC
  to get/list/watch podmonitors and servicemonitors when otelCIScrape is on.
- values.yaml: add otelContainerInsights.serviceMonitor.enabled and
  .podMonitor.enabled (default true).
- operator-deployment.yaml: supporting wiring.
…ategy

- CRD: allocationStrategy enum gains "per-node".
- cloudwatch-agent-custom-resource.yaml: set the TA allocationStrategy from
  values (defaults to per-node).
- values.yaml: prometheus.targetAllocator.allocationStrategy default per-node.
- _otel-container-insights-config.tpl: add set_node_name + promote_node_name
  transforms to the prometheuscr pipeline so each scraped series carries the
  scraping agent's node as @resource.k8s.node.name (from ${env:K8S_NODE_NAME}),
  enabling per-node verification and node enrichment.
…tep install

Bundle the community monitoring.coreos.com ServiceMonitor/PodMonitor CRDs as
templated, gated resources so the Target Allocator's prometheusCR scraping works
with no manual CRD prerequisite (matching the prometheus-operator experience).

- templates/prometheus-crds/{servicemonitor,podmonitor}-crd.yaml: templated + gated,
  skipped when an unmanaged copy already exists (Helm lookup) so a
  prometheus-operator-managed CRD is never clobbered; annotated
  helm.sh/resource-policy: keep so uninstall does not cascade-delete the CRs.
- values.yaml: prometheusCRDs.install (auto|always|never, default auto); auto
  bundles only when otelContainerInsights is enabled.
- _helpers.tpl: prometheusCRDsEnabled gating helper.
- tests/prometheus_crds_matrix.sh: helm template render tests for the gating.

The Target Allocator CRD-watch RBAC is deliberately not included here; it ships in
its own PR (aws-observability#327).
…ights.prometheusScrape

Collapse the coupled, scattered ServiceMonitor/PodMonitor scraping flags into a
single nested bundle so the common case is one decision: enable
otelContainerInsights and SM/PM scraping works end to end (CRDs bundled + Target
Allocator + prometheusCR + RBAC), with no other flags. This addresses review
feedback that the enablement surface spanned several non-obvious, interacting
flags (top-level prometheusCRDs, otelContainerInsights.serviceMonitor/podMonitor,
and the deep agent targetAllocator/prometheusCR knobs).

New shape, all under otelContainerInsights.prometheusScrape:
  enabled (single opt-out for scraping), allocationStrategy, crds.install
  (auto|always|never), serviceMonitor.enabled, podMonitor.enabled.

- values.yaml: add the prometheusScrape bundle; remove top-level prometheusCRDs;
  revert the deep agent prometheus.targetAllocator.allocationStrategy default to
  consistent-hashing (the CI path now sources per-node from the bundle, and the
  deep field only serves agents that enable the Target Allocator directly).
- _helpers.tpl: otelCIScrapeEnabled now gates solely on prometheusScrape.enabled;
  prometheusCRDsEnabled reads prometheusScrape.crds.install and, for auto, requires
  both otelContainerInsights.enabled and prometheusScrape.enabled (dig-based, so a
  missing sub-key is safe).
- cloudwatch-agent-custom-resource.yaml: compute allocationStrategy from the bundle
  on the CI path (deep agent field otherwise); read the monitor selector gates from
  the bundle.
- _otel-container-insights-config.tpl: gate the prometheuscr receiver and pipeline
  on prometheusScrape.enabled.
- prometheus-crds comments + tests/prometheus_crds_matrix.sh: new paths, plus
  coverage for prometheusScrape off, single-monitor-type, and the per-node default.

Clean rename with no aliasing: these keys have never shipped in a released chart
version, so there is no external contract to preserve.

Verified: helm lint clean; default / CI-on / scrape-off full renders clean;
prometheus_crds_matrix.sh 21/21 pass; prometheuscr receiver present when scrape
is on and absent when off.
The prior comment attributed a $-collapsing pass to Helm. Helm passes the text
through unchanged; the two $$->$ collapses happen in the agent's OTel confmap
stack (legacy expandconverter + the resolver's built-in ${...} expansion), so
$$$1 lands as the literal $1 regex backreference. Comment-only; render unchanged.
- "--auto-instrumentation-nodejs-image={{ template "auto-instrumentation-nodejs.image" . }}"
- "--target-allocator-image={{ template "target-allocator.image" (merge .Values.agent.prometheus.targetAllocator.image (dict "region" $.Values.region)) }}"
- "--feature-gates=operator.autoinstrumentation.multiinstrumentation,operator.autoinstrumentation.multiinstrumentation.skipcontainervalidation"
- "--feature-gates=operator.autoinstrumentation.multi-instrumentation,operator.autoinstrumentation.multi-instrumentation.skip-container-validation"

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same --feature-gates issue as #329: hyphenated multi-instrumentation IDs the pinned operator doesn't register, so it CrashLoopBackOffs. Could we fix it in #329 and rebase?

## Allocator's prometheusCR discovery. When otelContainerInsights.enabled is true,
## enabling this gives SM/PM scraping end-to-end (CRDs + Target Allocator +
## prometheusCR + RBAC). Only takes effect when otelContainerInsights.enabled is true.
prometheusScrape:

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The prometheusScrape bundle moves prometheusCRDs.install and the monitor flags to new prometheusScrape.* keys with no alias, so an existing --set prometheusCRDs.install=never is silently ignored. If we can't guarantee merge order, could we add aliases or a deprecation path?

- nonResourceURLs: ["/metrics"]
verbs: ["get"]
{{- if and (hasKey ($customAgent.prometheus.targetAllocator) "prometheusCR") $customAgent.prometheus.targetAllocator.prometheusCR.enabled }}
{{- if or (and (hasKey ($customAgent.prometheus) "targetAllocator") (hasKey ($customAgent.prometheus.targetAllocator) "prometheusCR") $customAgent.prometheus.targetAllocator.prometheusCR.enabled) $otelCIScrape }}

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here: the SM/PM rule is present but the customresourcedefinitions rule (in #327) isn't, so this branch needs #327 to land alongside for the target-allocator to list CRDs. Could we make sure they merge together under the same gate?

@@ -0,0 +1,1429 @@
{{- /*
PodMonitor CRD (monitoring.coreos.com/v1), pinned to prometheus-operator v0.91.0 to

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Header pins these CRDs to prometheus operator v0.91.0 but go.mod is on v0.92.0, so the API server prunes any field added between the two from a user's monitor. Could we refetch both from v0.92.0 (or fix the header)? Same in servicemonitor-crd.yaml.

@@ -6405,9 +6405,10 @@ spec:
allocationStrategy:

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

allocationStrategy gains per-node only in the crds/ copy, which Helm doesn't touch on upgrade, so upgrading a release whose CRD predates the enum makes the API server reject the rendered per-node. Could we move this CRD into templates/ and gate it like the bundled monitor CRDs?


printf "\n${Y}== Target Allocator CRD RBAC ==${N}\n"
# The TA needs customresourcedefinitions get;list;watch on the prometheus-CR path.
expect_count "CRD RBAC present when otelCI on" "$CRD_RBAC" 1 --set otelContainerInsights.enabled=true

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The CRD_RBAC assertions on lines 76 to 78 expect customresourcedefinitions RBAC that lands in a separate branch, so this branch renders zero and set -euo pipefail fails the whole suite. Could we move them into that branch or guard them until the rule exists?

otelContainerInsights.prometheusScrape.enabled are both true.
Returns the string "true" when CRDs should be rendered, empty otherwise.
*/}}
{{- define "amazon-cloudwatch-observability.prometheusCRDsEnabled" -}}

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

prometheusCRDsEnabled only matches always/never exactly, so a typo like nevr falls through and silently bundles the CRDs when otelCI is on. Could we add a fail branch for anything that isn't auto, always, or never?

the configured targetAgent, and at least one of serviceMonitor/podMonitor is enabled.
Accepts a dict with "agentName" (string) and "context" (root context $).
*/}}
{{- define "cloudwatch-agent.otelCIScrapeEnabled" -}}

@musa-asad musa-asad Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

otelCIScrapeEnabled no longer short circuits when both serviceMonitor.enabled and podMonitor.enabled are false, so prometheusScrape.enabled=true renders an idle target-allocator that bundles CRDs but discovers nothing. Could we reject that both false case and add a matrix assertion?

The Target Allocator watches the ServiceMonitor/PodMonitor CRDs to start/stop
informers as CRDs appear or disappear. Under the otelContainerInsights scraping
path the TA ClusterRole granted list/watch on the monitors but not on
customresourcedefinitions, so the operator-side CRD watch was denied, the CRD
gate never opened, and SM/PM discovery failed quietly. Add the read-only
get/list/watch grant on customresourcedefinitions beside the monitoring.coreos.com
rule under the same $otelCIScrape gate. Comment-only render change; gated off by
default.
This branch had flipped the operator --feature-gates to the hyphenated
multi-instrumentation / multi-instrumentation.skip-container-validation form.
The pinned operator (cloudwatch-agent-operator:3.5.0) registers the
unhyphenated IDs (operator.autoinstrumentation.multiinstrumentation and
.multiinstrumentation.skipcontainervalidation), so the hyphenated forms are
unrecognized; the manager parses gates with ExitOnError and CrashLoopBackOffs,
which stops all CR reconciliation and breaks the whole chart on install/upgrade.
Out of scope for the scraping work; restore the unhyphenated IDs to match main.
…ment

The transform/cw_k8s_ci_v0_set_scope_prometheuscr processor was defined
unconditionally, while its receiver and pipeline are guarded by
serviceMonitor.enabled OR podMonitor.enabled. With otelContainerInsights
enabled but both monitors disabled, the processor rendered orphaned (declared,
referenced by no pipeline), which the collector warns about. Wrap it in the
same guard so the receiver, pipeline and processor stay in sync.
The TA ClusterRole and ClusterRoleBinding rendered inside range .Values.agents
with a static object name, so if more than one agent satisfied the gate (which
$otelCIScrape widens), two identically named objects rendered and Kubernetes
applied last-write-wins, silently clobbering one. All TAs share one
ServiceAccount (target-allocator-service-acct), so a single ClusterRole/Binding
suffices: pre-scan agents into needsTA/needsCR and render exactly once, with the
SM/PM + customresourcedefinitions rules included when any agent needs discovery.
Renders once even with multiple matching agents; verified 1 object each.
….92.0

The bundled ServiceMonitor/PodMonitor CRDs were pinned to v0.91.0 while the
operator's go.mod (the Target Allocator's prometheus-operator client) pins
v0.92.0. Refetched both from the v0.92.0 tag and updated the headers, preserving
the Helm gating (prometheusCRDsEnabled + skip-if-exists) and metadata edits
(helm.sh/resource-policy: keep + chart labels). v0.92.0 adds no fields to SM/PM,
so no admission-time pruning; the real delta is version annotations and a URL
validation (minLength -> pattern) that now matches the operator's client.
The bundled CRD skip-if-exists guard only re-rendered (adopted/upgraded) an
existing CRD when meta.helm.sh/release-name matched. CRDs are cluster-scoped, so
a same-named release in a different namespace could cross-adopt one. Also require
meta.helm.sh/release-namespace to match.

Document the GitOps caveat: lookup is empty under helm template, so the bundled
CRD always applies and can overwrite an externally managed CRD; set
prometheusCRDs.install: never to opt out (that opt-out already exists).
The prometheus_crds_matrix.sh uses bash-only features but had no shebang; add
#!/usr/bin/env bash and the exec bit to match flag_matrix.sh. The SM_CRD/PM_CRD
grep patterns were unanchored with unescaped regex dots that could over-count;
anchor them with ^[[:space:]]* ... $ and escaped dots for exact metadata.name
matches. Counts unchanged (verified).
prometheusCRDsEnabled matched "always"/"never" exactly and let any other
value fall through to the otelContainerInsights.enabled branch, so a mistyped
opt-out (e.g. "nevr") failed open and silently bundled the CRDs when otelCI was
on. Split "auto" into its own branch and add a catch-all else that fails with a
clear message (matching the metricResolution sibling), so unknown values are
rejected at render time regardless of otelCI state.
…bundling

Re-sync onto the updated scraping-path branch to inherit the feature-gate ID
fix, the co-gated customresourcedefinitions RBAC rule (render-once ClusterRole/
Binding), the $$$1 comment correction, and the prometheuscr scope-processor
guard. Resolves the CRD-RBAC matrix assertion (now 11/0) and the feature-gate
blocker on this branch.
allocationStrategy was emitted under $taEnabled with a hardcoded default of
per-node, so it also flipped classic static-config Target Allocators (enabled
without otelContainerInsights) from consistent-hashing to per-node -- beyond
this PR's scope. Default per-node only on the $otelCIScrape path and
consistent-hashing otherwise, while still honoring an explicit
allocationStrategy override. Drop the redundant | default "per-node" (values.yaml
no longer hardcodes it; the template now defaults context-aware).
…output

The per-node pipeline note claimed end-to-end verification via target_node
matching k8s.node.name, but the chart only stamps k8s.node.name (the scraping
agent's own node); target_node is not emitted by the chart. Reword the note so
target_node is described as a prerequisite the user supplies via SM/PM
relabeling, and note that k8s.node.name is stamped unconditionally.
The prometheusCR pipeline ran set_node_name/promote_node_name, unconditionally
overwriting k8s.node.name with the scraping agent's own node. That is only
correct when per-node placement succeeds; under the consistent-hashing fallback
(cross-node target) it mislabels the metric, and it clobbers any node label the
target/relabeling already set. Per-node placement is verified via the Target
Allocator logs, so the stamp is not needed here -- remove both transforms from
the prometheusCR pipeline. The shared transform definitions stay for the
node-level pipelines (cadvisor, node_exporter, etc.) where the agent's node is
the metric's node.
…node-allocation

Re-sync aws-observability#330 onto the updated scraping-path branch: inherit the feature-gate ID
fix, co-gated CRD RBAC render-once ClusterRole/Binding, the $$$1 comment
correction, and the prometheuscr scope-processor guard, alongside aws-observability#330's per-node
work.
…bundling

Re-sync aws-observability#331 onto aws-observability#330 so the stack is linear (aws-observability#329 -> aws-observability#330 -> aws-observability#331): inherit
the per-node allocationStrategy scoping/gating, the SM/PM node-stamping removal,
and the context-aware allocationStrategy default, on top of aws-observability#331's CRD bundling.
…ation notice

The prometheusScrape restructure moved prometheusCRDs.install and the
serviceMonitor/podMonitor flags to otelContainerInsights.prometheusScrape.* with
no alias, so an existing --set prometheusCRDs.install=never (or the monitor
flags) was silently ignored on upgrade. Honor the legacy keys as fallbacks
(prometheusCRDsEnabled + new serviceMonitorEnabled/podMonitorEnabled helpers) so
existing overrides keep working regardless of merge/release order, and add a
NOTES.txt deprecation notice that fires only when a legacy key is set, pointing
to the new prometheusScrape.* key.
…cture

Re-sync aws-observability#332 onto the updated stack (aws-observability#329->aws-observability#330->aws-observability#331). Resolutions favor the
settled older-PR fixes applied onto aws-observability#332's prometheusScrape.* restructure:
- prometheusCRDsEnabled: keep aws-observability#331's explicit auto branch + invalid-value fail,
  gated on aws-observability#332's otelContainerInsights.enabled && prometheusScrape.enabled.
- allocationStrategy: use aws-observability#332's precomputed $allocationStrategy (per-node on the
  otelCI path via prometheusScrape.allocationStrategy, consistent-hashing for
  direct-TA) -- supersedes aws-observability#330's ternary while preserving its intent.
- Fix a stale old-key guard (otelContainerInsights.serviceMonitor.enabled) left by
  auto-merge in the prometheuscr scope-processor to prometheusScrape.enabled.
- Keep aws-observability#330's SM/PM node-stamping removal; port aws-observability#331's GitOps opt-out caveat into
  the new prometheusScrape.crds doc.
Verified: lint clean, CRD matrix 21/0, flag matrix 8/8.
wenegiemepraise added a commit to wenegiemepraise/helm-charts that referenced this pull request Jul 23, 2026
…ion-scraper-routing

Re-sync aws-observability#339 (scraper routing) onto the full updated stack. Preserve aws-observability#339's
annotation-routing feature (scraperRole: cluster-scraper) while adopting the
older/settled fixes: the prometheusScrape restructure, legacy-key aliases +
deprecation notice, the serviceMonitorEnabled/podMonitorEnabled helpers for the
SM/PM selector guards, the feature-gate/CRD-RBAC/render-once/v0.92.0-CRD fixes,
and per-node scoping. Verified: lint clean, CRD matrix 24/0, flag matrix 8/8,
scraperRole renders, legacy prometheusCRDs.install alias honored.
The legacy-key aliases in prometheusCRDsEnabled and the new
serviceMonitorEnabled/podMonitorEnabled helpers already keep existing
--set prometheusCRDs.install / serviceMonitor.enabled / podMonitor.enabled
overrides working across the prometheusScrape.* rename. The chart had no
NOTES.txt before this stack; adding one solely for deprecation warnings
introduced a chart-wide empty NOTES block for every user not using a legacy
key. Drop it and rely on the aliases, which are the substantive fix.
prometheusScrape.enabled=true with both serviceMonitor.enabled and
podMonitor.enabled false is contradictory: otelCIScrapeEnabled stayed true,
so an idle Target Allocator rendered and CRDs bundled while nothing was
discovered. Add validatePrometheusScrape, invoked from the always-rendered
custom-resource template, to fail loudly with guidance (enable a monitor or
set prometheusScrape.enabled=false). Add a matrix expect_fail assertion.
wenegiemepraise added a commit to wenegiemepraise/helm-charts that referenced this pull request Jul 24, 2026
…ion-scraper-routing

Propagates the both-false prometheusScrape validation (reject scrape.enabled
with no monitors).

# Conflicts:
#	charts/amazon-cloudwatch-observability/tests/prometheus_crds_matrix.sh
$needsCR granted the monitoring.coreos.com SM/PM read rule whenever any agent
had prometheusCR.enabled, even if that agent's targetAllocator was disabled, so
a stale prometheusCR.enabled over-granted. Mirror $needsTA by also requiring
targetAllocator.enabled, keeping the ClusterRole least-privilege. The otelCI
scrape path ($otelCIScrape) still grants it as before.
wenegiemepraise added a commit to wenegiemepraise/helm-charts that referenced this pull request Jul 24, 2026
…ion-scraper-routing

Propagates the needsCR least-privilege fix (require targetAllocator.enabled).
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.

2 participants