pkg/monitor: fix activity units and document Prometheus naming before release by cmcantalupo · Pull Request #209 · intel/goresctrl

cmcantalupo · 2026-09-23T23:19:10Z

Description

Follow-up to #192. This PR has fixes and documentation for pkg/monitor that came up while integrating it into the nri-plugins resctrl-mon plugin (containers/nri-plugins#757). They should land before the first tagged release that includes pkg/monitor, because one of them changes exported values.

Changes

AET activity units (pkg/monitor: report AET activity in nanofarads)
- The kernel reports activity in nanofarads, not farads as Documentation/filesystems/resctrl.rst currently states. A kernel documentation fix is in progress.
- Reading.Unit for activity is now UCUM nF, and ReadCounters still returns the raw kernel value.
- For OTel export the value is converted to the base unit. perf.activity keeps unit farads and observed values are scaled by 1e-9, so the Prometheus name stays perf_activity_farads_total.
- Scaling is applied after the monotonic accumulator, so accumulator state stays in kernel units.
- Per-counter export overrides live in a small otelExport table; TestMetaOTel covers the override and passthrough behavior.
Prometheus naming documentation (pkg/monitor: correct Prometheus naming documentation)
- The InstrumentName doc claimed that keeping _bytes makes mbm_total_bytes render as l3_mbm_total_bytes_total, and that pkg/rdt's l3.mbm.total renders incorrectly.
- The rendered name is actually chosen by the consumer's exporter translation strategy:
  - UnderscoreEscapingWithSuffixes gives l3_mbm_bytes_total. This strategy drops every total word from a counter name, so pkg/rdt's name renders the same way.
  - NoUTF8EscapingWithSuffixes gives l3.mbm.total.bytes_total.
- The docs now describe the mechanical file-to-instrument mapping and the strategy-dependent rendering, and recommend that consumers select a strategy explicitly.
Log noise
- A counter that was not discovered at registration was warned about on every collection for every group. It is now warned about once, with debug-level messages after that.
- RegisterOTelInstruments now warns when it discovers no counters. It still returns a no-op Registration and a nil error.
- Routine create, assign-PID and remove messages move from info to debug level. Adopting an existing group, reconcile orphan removal, and all warnings are unchanged.
Docs (doc/resctrl-mon.md)
- Teardown guidance: remove a pod's group only when its last sandbox is gone. Kubelet can create a new sandbox for the same pod UID and garbage-collect the old one while the pod keeps running.
- New OpenTelemetry Export section covering instrument names, kinds, units, exported values, and the Prometheus names under both strategies.

Changes since the first revision

Rebased on current main.
Dropped the cmd/rdt translation-strategy pin. go.mod: bump otel/prometheus deps #210 bumps the exporter to v0.68.0, whose default is already UnderscoreEscapingWithSuffixes.
warned is now a map[string]bool.
Trimmed tests. TestPrometheusNames was dropped because the strategy is not configurable in pkg/monitor, and with it the direct otlptranslator dependency. The log-counting and OTel fixture tests were dropped too. TestMetaOTel is added.

Compatibility notes

Exported perf.activity values are now 1e9 times smaller: they are correct farads, where previously they were nanofarads labelled as farads. The metric name is unchanged.
OTel instrument names and go.mod are unchanged.

Testing

go build ./..., go vet ./..., gofmt, golangci-lint run (0 issues), and go test -race ./....
The containers/nri-plugins resctrl-mon tests pass against this branch.

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

What changed in this PR

Updates pkg/monitor for correct AET activity units, explicit Prometheus naming, quieter logging, and release documentation.

Changes:

Converts kernel nanofarad activity values to farads for OTel export.
Documents/tests Prometheus naming strategies and configures cmd/rdt explicitly.
Updates logging, teardown guidance, dependencies, and OTel documentation.

File	Summary
`pkg/monitor/reading.go`	Updates activity units and OTel scaling.
`pkg/monitor/reading_test.go`	Tests raw activity values and units.
`pkg/monitor/otel.go`	Applies scaling, warning behavior, and naming documentation.
`pkg/monitor/otel_test.go`	Tests exports, warnings, and Prometheus names.
`pkg/monitor/monitor.go`	Lowers routine lifecycle logs to debug.
`go.mod`	Promotes `otlptranslator` to a direct dependency.
`doc/resctrl-mon.md`	Documents teardown and OTel/Prometheus behavior.
`cmd/rdt/main.go`	Selects an explicit Prometheus translation strategy.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

marquiz

Thanks @cmcantalupo for the update. Some comments/nits below.

After we get this in we should tag a new release.

marquiz · 2026-09-24T05:02:18Z

 				otelprom.WithNamespace(""),
 				otelprom.WithoutScopeInfo(),
 				otelprom.WithoutTargetInfo(),
+				otelprom.WithTranslationStrategy(otlptranslator.UnderscoreEscapingWithSuffixes),


This should be the default in later versions. I submitted #210 so that we could drop this change for good

Dropped; #210's exporter v0.68.0 defaults to UnderscoreEscapingWithSuffixes

marquiz · 2026-09-24T07:26:10Z

 		cfg:    cfg,
 		meter:  meter,
 		instrs: make(map[string]metric.Observable),
+		warned: make(map[string]struct{}),


I'd suggest to change this to map[string]bool, makes the code using this more fluent and readable

Suggested change

warned: make(map[string]struct{}),

warned: make(map[string]bool),

Made the change, thanks for the suggestion.

marquiz · 2026-09-24T07:29:11Z

+		want     map[string]string
+	}{
+		{
+			strategy: otlptranslator.UnderscoreEscapingWithSuffixes,


Do we need/want to test every strategy, is it configurable?

Not configurable in pkg/monitor, since the strategy belongs to the consumer's exporter. Dropped the test; the docs still describe both renderings.

marquiz · 2026-09-24T07:34:04Z

+	assert.Equal(t, int32(1), h.n.Load())
+}
+
+func TestObserve_ActivityExportedInFarads(t *testing.T) {


nit: The new unit tests feel like a bit too much AI verifying it's specific output. The warn counter thingie feels like yeah, maybe ok but a lot of stuff for a human reader to read through. The farad thingie like if we do this could it be a bit more generic testing the general otelExport scaling concept. Thoughts?

Agreed. Removed the warn-counter and fixture tests and added a small generic TestMetaOTel for the otelExport override/passthrough.

The kernel reports the AET "activity" counter in nanofarads, not farads as the resctrl documentation currently states (a documentation fix is in progress). Reading.Unit for activity was "farads", mislabeling the raw value by a factor of 1e9. Record the raw unit as UCUM "nF" so ReadCounters stays faithful to the kernel value. For OTel export, convert to the base unit: the perf.activity instrument keeps unit "farads" (otlptranslator has no suffix mapping for UCUM "F", so "farads" keeps the Prometheus name perf_activity_farads_total) and observed values are scaled by 1e-9. Scaling is applied after the monotonic accumulator so its state stays in kernel units. Signed-off-by: Christopher M. Cantalupo <christopher.m.cantalupo@intel.com>

The InstrumentName documentation claimed that keeping the _bytes suffix makes the OTel Prometheus bridge render mbm_total_bytes as l3_mbm_total_bytes_total, and that pkg/rdt's l3.mbm.total renders incorrectly as l3_mbm_bytes_total. The rendered name is actually chosen by the consumer's exporter translation strategy: UnderscoreEscapingWithSuffixes -> l3_mbm_bytes_total NoUTF8EscapingWithSuffixes -> l3.mbm.total.bytes_total The underscore strategy removes every "total" word from a counter name before appending _total, so pkg/rdt's name renders identically. Older exporter releases derived their default strategy from model.NameValidationScheme (UTF-8, hence the NoUTF8 form, which a legacy scrape escapes to l3_mbm_total_bytes_total); newer releases default to the underscore strategy. Rewrite the documentation to describe the mechanical file-to-instrument mapping and the strategy-dependent rendering, and recommend that consumers select a strategy explicitly. Signed-off-by: Christopher M. Cantalupo <christopher.m.cantalupo@intel.com>

observe logged a warning for every reading of a counter that was not discovered at registration, i.e. once per group per collection. With frequent Prometheus scrapes and many groups this floods the log. Warn the first time an undiscovered instrument name is seen and log subsequent occurrences at debug level. Signed-off-by: Christopher M. Cantalupo <christopher.m.cantalupo@intel.com>

RegisterOTelInstruments silently returned a no-op Registration when the resctrl root had no readable mon_data counters (for example when resctrl is mounted without monitoring support), leaving the consumer exporting nothing with no indication why. Log a warning naming the scanned mon_data path. The call still succeeds and returns a no-op Registration, so the behavior stays compatible. Signed-off-by: Christopher M. Cantalupo <christopher.m.cantalupo@intel.com>

Creating a group, assigning a PID and removing a group are routine per-workload operations that consumers already log in their own terms (for example per pod). Logging them at info level in the library duplicates that output and ignores the consumer's chosen verbosity. Log these three events at debug level. Adopting a pre-existing group and reaping an orphan in Reconcile stay at info level, and all warnings are unchanged. Signed-off-by: Christopher M. Cantalupo <christopher.m.cantalupo@intel.com>

- Teardown: remove a pod's group only when its last sandbox is gone. Kubelet can create a new sandbox for the same pod UID and garbage-collect the old one while the pod keeps running, so removing the group on every sandbox removal would delete a live group. - Add an OpenTelemetry Export section listing each counter's instrument name, kind, exported unit and value (activity is exported in farads, scaled from the kernel's nanofarads). - Document the Prometheus names under both exporter translation strategies and recommend selecting a strategy explicitly. Signed-off-by: Christopher M. Cantalupo <christopher.m.cantalupo@intel.com>

cmcantalupo · 2026-09-24T22:43:55Z

Thanks for the quick feedback @marquiz. I think I have addressed your comments.

marquiz

Thanks @cmcantalupo, looks nice and clean to me now 👍

LGTM

kad requested review from kad, klihub and marquiz and a lite review from Copilot September 24, 2026 07:09

Copilot started reviewing on behalf of kad September 24, 2026 07:09 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

marquiz reviewed Sep 24, 2026

View reviewed changes

cmcantalupo added 6 commits September 24, 2026 15:34

cmcantalupo force-pushed the monitor-pretag branch from d3f316c to 57ea66f Compare September 24, 2026 22:41

cmcantalupo requested a review from marquiz September 24, 2026 22:43

marquiz approved these changes Sep 25, 2026

View reviewed changes

marquiz merged commit bb78a6c into intel:main Sep 25, 2026

Navigation Menu

pkg/monitor: fix activity units and document Prometheus naming before release - #209

pkg/monitor: fix activity units and document Prometheus naming before release#209
marquiz merged 6 commits into
intel:mainfrom
cmcantalupo:monitor-pretag

cmcantalupo commented Sep 23, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

marquiz left a comment

Uh oh!

marquiz Sep 24, 2026

Uh oh!

cmcantalupo Sep 24, 2026

Uh oh!

marquiz Sep 24, 2026

Uh oh!

cmcantalupo Sep 24, 2026

Uh oh!

marquiz Sep 24, 2026

Uh oh!

cmcantalupo Sep 24, 2026

Uh oh!

marquiz Sep 24, 2026

Uh oh!

cmcantalupo Sep 24, 2026

Uh oh!

cmcantalupo commented Sep 24, 2026

Uh oh!

marquiz left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	warned: make(map[string]struct{}),
	warned: make(map[string]bool),

Conversation

cmcantalupo commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Changes since the first revision

Compatibility notes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmcantalupo commented Sep 24, 2026

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cmcantalupo commented Sep 23, 2026 •

edited

Loading