feat: add node telemetry support by mselim00 · Pull Request #304 · aws/eks-node-monitoring-agent

mselim00 · 2026-10-14T21:57:13Z

Description of changes:

This adds support posting metrics scraped from local daemons and the agent itself to an in-cluster telemetry API. The new feature is disabled by default, but can be enabled through a dedicated sub-section in the existing monitor-config ConfigMap. Enabling telemetry leads the agent to, at the configured interval:

Scrape local configured targets at the configured endpoints and timeout
Collecting the agent's own conditions
Aggregate all of the findings into a "report" that it sends to the in-cluster API

This is a minimal implementation of the feature intended to span the necessary functionality. There are several TODOs scattered throughout to refine the logic, including:

Bounding the report's size
Comprehensive validation of the provided telemetry config
Limiting the agent's in-process collection to a configured catalog
Fetching the AMI ID from IMDS, and including that as an attribute in the report
Scraping non-fatal NMA health events and metrics

Testing Done:

Deployed an API server to to handle the posted metrics. Confirmed that:

The default helm chart does not lead NMA to post any metrics.
Configuring monitors in the default ConfigMap (without enabling telemetry) renders correctly and NMA starts
Editing the monitor ConfigMap to enable telemetry and then rolling out a restart leads agents to start reporting scraped metrics and NMA conditions (mocked a condition by injecting a fault in a running instance of the dcgm server on a GPU instance)
Restarting NMA still leads the agent to report at the same 5 minute interval, since the interval is calculated only based on the node's name

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Adds an MVP for posting metrics scraped from local daemons and the agent itself to a central in-cluster node telemetry API.

cdirubbio · 2026-10-19T21:16:36Z

 		close(registered)

+		if monitorConfig.TelemetryEnabled() {
+			go runTelemetry(ctx, logger.WithName("telemetry"), monitorConfig.Telemetry, monitorCfg, monitoringKubeClient, monitorMgr)


can we switch this to use serviceaccount in rbac rather than node in rbac? I'll start a thread internally as to why

zac-nixon · 2026-10-20T00:10:29Z

+)
+
+func runTelemetry(ctx context.Context, logger logr.Logger, cfg *telemetryconfig.Config, restCfg *rest.Config, kubeClient client.Client, source telemetry.FatalSource) {
+	sender, err := nodetelemetry.NewClient(restCfg)


This effectively renders telemetry off for the lifetime of this NMA process. We will want to be more resilient here and periodically retry construction of the client.

there's no retryable error in the call chain AFAICT: nodetelemetry.NewClient() -> rest.HTTPClientFor -> rest.TransportFor -> config.TransportConfig and transport.New(), which both error on static validation. in practice, this path can't really error since we're reusing the same config controller runtime manager was created with and went through the same call chain for.

I'd went with a fail-open approach with a log line for that edge case so that NMA could still monitor, but perhaps failing closed makes more sense here so that if this can ever produce an error in the future, it could be caught and stopped in a DaemonSet update rollout rather than letting every node silently stop reporting

i think fail open approach may work for now, but it'll probably bite us down the road if we ever ship a bug that breaks this telemetry thread.

fair, I think this could actually be entirely avoided by just re-using the manager's already constructed client since we're already re-using its config. done in 4570f7b, lmk what you think

this should be equivalent to before since client-go's transport is a cached lookup based on the config (link), and the config used between the controller runtime manager and the telemetry manager was already the same for all the keyed fields

zac-nixon · 2026-10-20T00:11:46Z

+	if err := wait.PollUntilContextCancel(ctx, 30*time.Second, true, func(ctx context.Context) (bool, error) {
+		return kubeClient.Get(ctx, types.NamespacedName{Name: hostname}, &node) == nil, nil
+	}); err != nil {
+		return


same concern, we should retry here periodically instead of just turning off telemetry.

this one actually does poll indefinitely because the context passed here is the manager's, which is only canceled on shutdown, though telemetry does end up being blocked until the node object can be fetched.

more broadly, on this note, I was thinking one of the following approaches could be better :

refactoring it into a generic runtime context bootstrapper, like the HybridNodesBootstrapper but ran in the background until it can fetch the node object, that way telemetry can be started earlier and attributes can be added into the reports as they are discovered

dropping the k8s Get() API call entirely by taking a hard dependency on IMDS for all attributes. the kubeletVersion cannot be fetched this way, but it can already be inferred from the AMI ID (if it's a public AMI). alternative local options for the kubelet version could be kubelet --version (if permissions allow) or reading /etc/eks/kubelet-version.txt on AL2023 AMIs link, which we could also get added to BR

+1 to using IMDS we already use elsewhere in NMA, so it's a not a new dependency. having a single source of truth is a nice property too.

that way telemetry can be started earlier and attributes can be added into the reports as they are discovered

I am not sure about, it may be confusing or provide wrong metrics if we lazily load these attributes. For example, getting some metrics without the kubelet version would potentially lead to incomplete metrics.

IMDS it is then, that also addresses a I was also seeing a race condition that particularly impacted Auto (NMA as a system daemon) where the first fetched Node object may not include the desired labels since they're applied post-registration by CP controllers. I'll make that change

I made this change in 4cb9fc7:

k8s version is still through a k8s Get(), should be safe & quick since the kubelet version should be in the first version of the node object that the kubelet creates

runtime.GOARCH for the host arch: should hold true except for the unlikely situation where NMA's ran in an emulated environment. could use kubernetes.io/arch, which is set by kubelet in the initial node object using the same source: link, but I'd prefer to avoid using the k8s API except where it's actually needed to make future cleanups easier

all else from IMDS. IMDS checks are skipped on hybrid nodes, so those attributes (like instance id and AZ) are left empty. hybrid nodes are expected to use their node name as their instance ID in the provider ID (link), so there's not much value add for these additional attributes for hybrid

zac-nixon · 2026-10-20T00:13:13Z

+func NodeAttrsFromNode(node *corev1.Node) nodetelemetry.NodeAttrs {
+	return nodetelemetry.NodeAttrs{
+		NodeName:       node.Name,
+		HostID:         InstanceID(node.Spec.ProviderID),


I don't think this is guaranteed to be the the instance-id. We should be defensive here for self-hosted NMA

for self-hosted/hybrid nodes, I don't know if it's safe to take this field, so I've currently left it to drop the instance ID from the report. as far as I know, there's nothing that would prevent a self-provided instance from having the same ID as an EC2 instance part of the same cluster. this should be guaranteed as the instance ID for cloud-provider-aws/ccm managed ec2 instances though

I think it's just a bit misleading rn, because if I have an "abnormal" provider ID then I set the HostID to "". Which is probably correct.

I think 4cb9fc7 makes this a bit more readable by only adding the attributes when they can be resolved. assuming this comment was on the InstanceId() function returning an empty string when the ID doesn't match the expected pattern

zac-nixon · 2026-10-20T00:28:43Z

+	md := pmetric.NewMetrics()
+	rm := md.ResourceMetrics().AppendEmpty()
+	a := rm.Resource().Attributes()
+	for k, v := range map[string]string{


nit: you can just a singleton instead of re-creating this map everytime.

agreed on this in the current format, I'm going to hold off on this though pending your thoughts on the point about starting telemetry early with a subset of attributes. that would benefit from re-constructing the attributes on each report

done in 4cb9fc7, there's now a single call to construct a new "Resource" which is used across all reports

zac-nixon · 2026-10-20T00:33:23Z

+	}
+}
+
+func (b *Baselines) Unreachable() {


I don't follow what unreachable means here.

I went ahead and renamed this in db944af, it's used as a catch-all for all scrape failures so I renamed it to that. it covers everything from us dropping the scrape because the response is too large to the scrape target being down/unresponsive

zac-nixon · 2026-10-20T00:53:35Z

+	NotReportingAttrReason = "reason"
+)
+
+const MaxPointsPerMetric = 50


Shouldn't this be gathered from the metric configuration rather than hardcoded?

I intended this as the client-side limit to match what the API's expected to accept, so it also applies as a ceiling to the maxSeries value defined in the user-configuration. I also made an update to make that ceiling explicit at config assembly rather than just at scrape time, done in 5a9ee2c. let me know if that makes sense

zac-nixon · 2026-10-20T01:58:34Z

+}
+
+func NewClient(cfg *rest.Config) (*Client, error) {
+	hc, err := rest.HTTPClientFor(cfg)


Do we need to add a timeout to the rest config?

that would probably be prudent to add, but I'm not sure if it's necessary. none of the rest configs used throughout have a configured timeout, including the default one generated for the controller-runtime. I'm thinking that's b/c the default configured http health check in client go sets a 30s ReadIdleTimeout and a 15s PingTimeout(link), so a dropped connection would be caught within about 45s. the kube-apiserver handles a stuck request by returning a 504 after a 60s timeout

for completeness, I added a 75s timeout in 8834820. that should never take effect in the expected path, but an API server configured with a higher timeout may open up this path. lmk what you think

zac-nixon · 2026-10-20T01:59:45Z

+		for _, s := range series {
+			key := seriesKey(metric, s.Labels)
+			seen[key] = true
+			base, ok := b.series[key]


Reset detection compares the latest counter against the last acknowledged baseline, rather than the last observed value.

Step | Counter | Behavior -- | -- | -- Establish baseline | 10 | Baseline becomes 10 Scrape; upload returns 503 | 100 | Delta 90 held; baseline remains 10 Process resets; next scrape | 20 | Incorrectly reports delta 10

The observed decrease from 100 to 20 should trigger reset handling. Instead, it is missed because 20 exceeds the acknowledged baseline of 10. I reproduced this with a focused regression test.
Fix: track the last observed value separately from the acknowledged baseline, and explicitly handle pending deltas across resets.

good catch, added in lastScraped and carry fields to track the last scraped value and the delta not acknowledged by the extension at a reset, respectively. I also added a test case for this specific example to ensure it measures a reset as starting at 0 and reports 110, done in d07b793

edit: oops, forgot we'd want to establish a new baseline after a reset rather than assuming a 0-start. fixed that in eb2d2ca so it'll report 90 instead, and if it increases from 20 again it'll add that in the next delta (it increments an extra 5 in the test case)

cdirubbio · 2026-10-26T18:36:04Z

+}
+
+// addComputeType sets ComputeType from the runtime tags
+func addComputeType(attrs *nodetelemetry.NodeAttrs, tags []string) {


can we stay consistent and set compute type for regular eks-ec2 nodes as well?

we can, I just felt less comfortable giving that one a label since it's not really tied to a check, but rather the omission of other categories. eks-auto specifically means the node is an auto mode one and hybrid means it's a hybrid node, those two things are mutually exclusive. eks-ec2 would currently just be the case where neither of those labels were applied, and that's totally fine, but it's different from the others since that definition isn't tied to anything specific and could therefore change over time, e.g. if NMA identified and tagged nodes managed by EKS MNG

I see your point yea. It would be tied to visibility service-side atm but if we are going to have a change like you mentioned, worth holdign off for now

mselim00 requested a review from a team as a code owner October 14, 2026 21:57

cdirubbio reviewed Oct 14, 2026

View reviewed changes

Comment thread pkg/nodetelemetry/report.go

Pavani-Panakanti requested a balanced review from Copilot October 14, 2026 23:17

mselim00 force-pushed the telemetry branch 2 times, most recently from 44e02e2 to 2265070 Compare October 14, 2026 23:29

Copilot AI reviewed Oct 14, 2026

Copilot stopped reviewing on behalf of Pavani-Panakanti due to an error October 15, 2026 00:01
The job was not acquired by a runner even after multiple attempts.

Pavani-Panakanti requested a balanced review from Copilot October 15, 2026 02:02

Copilot AI reviewed Oct 15, 2026

feat: add node telemetry support

5a933f3

Adds an MVP for posting metrics scraped from local daemons and the agent itself to a central in-cluster node telemetry API.

mselim00 force-pushed the telemetry branch from 2265070 to 5a933f3 Compare October 16, 2026 15:23

cdirubbio reviewed Oct 19, 2026

View reviewed changes