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

doc/resctrl-mon.md

@@ -105,6 +105,9 @@ mgr.AssignPID(podUID, pid)
 // Tie the group's lifetime to the pod, not to individual containers — a
 // container restart reuses the sandbox, and releasing the RMID early would
 // hand the replacement a fresh RMID whose counters carry residual values.
+// Remove only when the pod's 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.
 mgr.Remove(podUID)
 
 // Crash recovery: remove orphaned groups not in the live set
@@ -114,11 +117,53 @@ mgr.Reconcile(liveKeys)
 readings, _ := mgr.ReadCounters(podUID)
 for _, r := range readings {
     // r.Kind: monitor.Gauge (instantaneous) or monitor.Cumulative (monotonic counter)
-    // r.Unit: UCUM where available ("By", "J"), otherwise descriptive ("farads", "")
+    // r.Unit: UCUM unit of the raw kernel value where known ("By", "J", "nF"), otherwise ""
     fmt.Printf("%s/%s = %f (%v, %s)\n", r.Domain, r.Name, r.Value, r.Kind, r.Unit)
 }
 ```
 
+## OpenTelemetry Export
+
+`Manager.RegisterOTelInstruments` registers one observable instrument per
+counter discovered under the resctrl root's `mon_data`. Instrument names are
+derived mechanically from the counter files (see `InstrumentName`), cumulative
+counters pass through a monotonic accumulator, and every data point carries
+`domain.id` and `domain.name` attributes.
+
+| resctrl file | Instrument | Kind | Unit | Exported value |
+|---|---|---|---|---|
+| `mon_L3_*/llc_occupancy` | `l3.llc.occupancy` | gauge | `By` | raw |
+| `mon_L3_*/mbm_local_bytes` | `l3.mbm.local.bytes` | counter | `By` | raw |
+| `mon_L3_*/mbm_total_bytes` | `l3.mbm.total.bytes` | counter | `By` | raw |
+| `mon_PERF_PKG_*/core_energy` | `perf.core.energy` | counter | `J` | raw |
+| `mon_PERF_PKG_*/activity` | `perf.activity` | counter | `farads` | raw nanofarads × 1e-9 |
+| other `mon_PERF_PKG_*` files | `perf.<name>` | counter if known, else gauge | none | raw |
+
+`ReadCounters` always returns the raw kernel value; for `activity` its `Unit`
+is `nF`.
+
+Prometheus names are produced by the consumer's exporter and depend on its
+translation strategy:
+
+| Instrument | `UnderscoreEscapingWithSuffixes` | `NoUTF8EscapingWithSuffixes` |
+|---|---|---|
+| `l3.llc.occupancy` | `l3_llc_occupancy_bytes` | `l3.llc.occupancy_bytes` |
+| `l3.mbm.local.bytes` | `l3_mbm_local_bytes_total` | `l3.mbm.local.bytes_total` |
+| `l3.mbm.total.bytes` | `l3_mbm_bytes_total` | `l3.mbm.total.bytes_total` |
+| `perf.core.energy` | `perf_core_energy_joules_total` | `perf.core.energy_joules_total` |
+| `perf.activity` | `perf_activity_farads_total` | `perf.activity_farads_total` |
+
+The underscore strategy drops the word `total` from `l3.mbm.total.bytes`.
+Older `go.opentelemetry.io/otel/exporters/prometheus` releases choose their
+default strategy from the global name validation scheme, so select one
+explicitly:
+
+```go
+exp, err := prometheus.New(
+    prometheus.WithTranslationStrategy(otlptranslator.UnderscoreEscapingWithSuffixes),
+)
+```
+
 ## RMID Exhaustion
 
 Each mon_group consumes one RMID from a pool with a size that is platform

pkg/monitor/monitor.go

@@ -294,7 +294,7 @@ func (m *Manager) EnsureGroup(key, rdtClass string) (*Group, error) {
 			return nil, fmt.Errorf("failed to create mon_group %s: %w", monGroupDir, err)
 		}
 	} else {
-		log().Info("created mon_group", "key", key, "dir", monGroupDir)
+		log().Debug("created mon_group", "key", key, "dir", monGroupDir)
 	}
 
 	// Bump the monotonic generation on every (re)creation so a remove→recreate
@@ -363,7 +363,7 @@ func (m *Manager) AssignPID(key string, pid int) error {
 	if err := f.Close(); err != nil {
 		return fmt.Errorf("failed to write pid %d for key %s: %w", pid, key, err)
 	}
-	log().Info("assigned PID to mon_group", "key", key, "pid", pid)
+	log().Debug("assigned PID to mon_group", "key", key, "pid", pid)
 	return nil
 }
 
@@ -485,7 +485,7 @@ func (m *Manager) Remove(key string) error {
 	}
 
 	delete(m.entries, key)
-	log().Info("removed mon_group", "key", key, "dir", e.dir)
+	log().Debug("removed mon_group", "key", key, "dir", e.dir)
 	return nil
 }
 

pkg/monitor/otel.go

@@ -107,8 +107,8 @@ func (r *Registration) Close() error {
 //	mon_L3_00/llc_occupancy          → l3.llc.occupancy           (unit: By)
 //	mon_L3_00/mbm_local_bytes        → l3.mbm.local.bytes         (unit: By)
 //	mon_L3_00/mbm_total_bytes        → l3.mbm.total.bytes         (unit: By)
-//	mon_PERF_PKG_00/core_energy      → perf.core.energy            (unit: J)
-//	mon_PERF_PKG_00/activity         → perf.activity                (unit: farads)
+//	mon_PERF_PKG_00/core_energy      → perf.core.energy           (unit: J)
+//	mon_PERF_PKG_00/activity         → perf.activity              (unit: farads; kernel nF × 1e-9)
 //	mon_PERF_PKG_00/c1_res           → perf.c1.res
 //	mon_PERF_PKG_00/c6_res           → perf.c6.res
 //	mon_PERF_PKG_00/uops_retired     → perf.uops.retired
@@ -131,7 +131,8 @@ func (r *Registration) Close() error {
 // L3 instrument names are similar to pkg/rdt's RegisterOpenTelemetryInstruments
 // but preserve the _bytes counter suffix (e.g. l3.mbm.total.bytes vs
 // pkg/rdt's l3.mbm.total). This maintains backward compatibility with the
-// kernel counter file names as a mechanical derivation.
+// kernel counter file names as a mechanical derivation. See InstrumentName for
+// how Prometheus exporters render these names.
 //
 // Each metric carries a "domain.id" attribute with the numeric instance
 // (e.g. "00") and a "domain.name" attribute with the full domain directory
@@ -157,6 +158,7 @@ func (m *Manager) RegisterOTelInstruments(meter metric.Meter, opts ...OTelOption
 		cfg:    cfg,
 		meter:  meter,
 		instrs: make(map[string]metric.Observable),
+		warned: make(map[string]bool),
 		accum:  newOTelAccumulator(),
 	}
 
@@ -177,6 +179,7 @@ type otelObserver struct {
 
 	mu     sync.Mutex
 	instrs map[string]metric.Observable // instrName → instrument
+	warned map[string]bool              // undiscovered instrument names already warned about
 	accum  *otelAccumulator
 	reg    metric.Registration // batch callback registration; nil if none
 }
@@ -187,7 +190,8 @@ type otelObserver struct {
 // not read — so instruments are registered even when the kernel reports
 // temporary placeholder values like "Unavailable".
 func (o *otelObserver) discoverAndRegister() error {
-	counters, err := discoverCounters(filepath.Join(o.mgr.root, "mon_data"))
+	monDataPath := filepath.Join(o.mgr.root, "mon_data")
+	counters, err := discoverCounters(monDataPath)
 	if err != nil {
 		return err
 	}
@@ -202,7 +206,8 @@ func (o *otelObserver) discoverAndRegister() error {
 			continue
 		}
 		seen[name] = struct{}{}
-		instr, err := o.createInstrument(name, metaKind(c.counter), metaUnit(c.counter))
+		unit, _ := metaOTel(c.counter)
+		instr, err := o.createInstrument(name, metaKind(c.counter), unit)
 		if err != nil {
 			return err
 		}
@@ -211,6 +216,7 @@ func (o *otelObserver) discoverAndRegister() error {
 	}
 
 	if len(observables) == 0 {
+		log().Warn("otel: no resctrl counters discovered; nothing will be exported", "path", monDataPath)
 		return nil
 	}
 
@@ -305,9 +311,17 @@ func (o *otelObserver) observe(ctx context.Context, obs metric.Observer) {
 			instrName := InstrumentName(r.Domain, r.Name)
 			o.mu.Lock()
 			instr := o.instrs[instrName]
+			warned := o.warned[instrName]
+			if instr == nil && !warned {
+				o.warned[instrName] = true
+			}
 			o.mu.Unlock()
 			if instr == nil {
-				log().Warn("otel: unknown counter skipped (not discovered at registration)",
+				logUnknown := log().Debug
+				if !warned {
+					logUnknown = log().Warn
+				}
+				logUnknown("otel: unknown counter skipped (not discovered at registration)",
 					"instrument", instrName, "domain", r.Domain, "counter", r.Name)
 				continue
 			}
@@ -316,6 +330,9 @@ func (o *otelObserver) observe(ctx context.Context, obs metric.Observer) {
 			if r.Kind == Cumulative {
 				val = o.accum.monotonic(key, r.Domain, r.Name, val, g.Gen())
 			}
+			// Scale after accumulating so accumulator state stays in kernel units.
+			_, scale := metaOTel(r.Name)
+			val *= scale
 
 			attrs := make([]attribute.KeyValue, 0, len(groupAttrs)+2)
 			attrs = append(attrs, attribute.String("domain.id", DomainInstance(r.Domain)))
@@ -343,21 +360,22 @@ func (o *otelObserver) observe(ctx context.Context, obs metric.Observer) {
 // --- Naming helpers (exported for use by callers building custom export) ---
 
 // InstrumentName derives the OTel instrument name from a resctrl domain
-// directory name and counter file name.
+// directory name and counter file name: the domain's resource prefix followed
+// by the counter file name with "_" replaced by ".". The mapping is mechanical
+// (the _bytes suffix is kept) so every instrument name identifies its resctrl
+// file.
+//
+// Prometheus names are chosen by the consumer's exporter, not by this package,
+// and depend on its translation strategy. For the counter l3.mbm.total.bytes
+// (unit By):
 //
-// The counter file name is converted to dot-separated segments and prepended
-// with the domain's resource prefix. The _bytes suffix is preserved (not
-// stripped) so that the OTel→Prometheus bridge's unit-suffix deduplication
-// produces correct names without colliding with the counter _total suffix
-// convention.
+//	UnderscoreEscapingWithSuffixes → l3_mbm_bytes_total
+//	NoUTF8EscapingWithSuffixes     → l3.mbm.total.bytes_total
 //
-// NOTE: This intentionally diverges from pkg/rdt's RegisterOpenTelemetryInstruments
-// which uses names like "l3.mbm.total" (stripping _bytes). That approach
-// produces incorrect Prometheus names via the OTel bridge: the bridge treats
-// the trailing "total" as a counter suffix, yielding "l3_mbm_bytes_total"
-// instead of the expected "l3_mbm_total_bytes_total". By preserving _bytes in
-// the OTel name, the bridge sees the unit is already present and only appends
-// _total for counters, producing the correct final name.
+// The underscore strategy removes every "total" word from a counter name
+// before appending _total, so pkg/rdt's l3.mbm.total renders the same way.
+// Consumers should select a strategy explicitly, e.g. with
+// prometheus.WithTranslationStrategy(otlptranslator.UnderscoreEscapingWithSuffixes).
 //
 // Examples:
 //

pkg/monitor/reading.go

@@ -46,7 +46,7 @@ type Reading struct {
 	Name   string      // counter file name, e.g. "llc_occupancy", "core_energy"
 	Value  float64     // parsed value (float to cover core_energy/activity)
 	Kind   ReadingKind // Gauge or Cumulative
-	Unit   string      // UCUM unit where available (e.g. "By", "J"), otherwise descriptive (e.g. "farads")
+	Unit   string      // UCUM unit of Value where known (e.g. "By", "J", "nF"), otherwise ""
 }
 
 // readingMeta maps known counter names to their kind and unit.
@@ -58,9 +58,9 @@ var readingMeta = map[string]struct {
 	"mbm_total_bytes": {Cumulative, "By"},
 	"mbm_local_bytes": {Cumulative, "By"},
 	"core_energy":     {Cumulative, "J"},
-	// activity accumulates dynamic capacitance; its rate of change is the
-	// workload's dynamic capacitance (Cdyn)
-	"activity":             {Cumulative, "farads"},
+	// activity accumulates dynamic capacitance in nanofarads; its rate of
+	// change is the workload's dynamic capacitance (Cdyn)
+	"activity":             {Cumulative, "nF"},
 	"c1_res":               {Cumulative, ""},
 	"c6_res":               {Cumulative, ""},
 	"uops_retired":         {Cumulative, ""},
@@ -163,3 +163,22 @@ func metaUnit(name string) string {
 	}
 	return ""
 }
+
+// otelExport overrides the exported OTel unit and scale for counters whose raw
+// kernel unit is not a base unit.
+var otelExport = map[string]struct {
+	unit  string
+	scale float64
+}{
+	// "farads" rather than UCUM "F": otlptranslator has no farad suffix mapping.
+	"activity": {"farads", 1e-9},
+}
+
+// metaOTel returns the OTel instrument unit for a counter and the factor that
+// converts its raw value into that unit.
+func metaOTel(name string) (unit string, scale float64) {
+	if o, ok := otelExport[name]; ok {
+		return o.unit, o.scale
+	}
+	return metaUnit(name), 1
+}

pkg/monitor/reading_test.go

@@ -50,8 +50,10 @@ func TestReadCounters_MultiDomain(t *testing.T) {
 	// Build a lookup map: domain/name -> value
 	type key struct{ domain, name string }
 	got := make(map[key]float64)
+	units := make(map[key]string)
 	for _, r := range readings {
 		got[key{r.Domain, r.Name}] = r.Value
+		units[key{r.Domain, r.Name}] = r.Unit
 	}
 
 	// L3 domain — integer values parse as float64.
@@ -62,6 +64,11 @@ func TestReadCounters_MultiDomain(t *testing.T) {
 	assert.InDelta(t, 54446119.644974, got[key{"mon_PERF_PKG_00", "core_energy"}], 0.001)
 	assert.InDelta(t, 1042.371582, got[key{"mon_PERF_PKG_00", "activity"}], 0.001)
 
+	// Readings carry the raw kernel value and unit (activity is nanofarads).
+	assert.Equal(t, "J", units[key{"mon_PERF_PKG_00", "core_energy"}])
+	assert.Equal(t, "nF", units[key{"mon_PERF_PKG_00", "activity"}])
+	assert.Equal(t, "By", units[key{"mon_L3_00", "mbm_total_bytes"}])
+
 	// mon_L3_01 has only "Unavailable" — no reading should be emitted for it.
 	_, hasL301 := got[key{"mon_L3_01", "llc_occupancy"}]
 	assert.False(t, hasL301, "non-numeric 'Unavailable' should be skipped")
@@ -197,3 +204,16 @@ func TestMetaKind_UnknownDefaultsToGauge(t *testing.T) {
 	// accumulator rather than risk corrupting a real gauge.
 	assert.Equal(t, Gauge, metaKind("some_future_counter"))
 }
+
+func TestMetaOTel(t *testing.T) {
+	// Counters listed in otelExport are exported in its unit and scale.
+	for name, want := range otelExport {
+		unit, scale := metaOTel(name)
+		assert.Equal(t, want.unit, unit, name)
+		assert.Equal(t, want.scale, scale, name)
+	}
+	// Other counters are exported in their raw unit, unscaled.
+	unit, scale := metaOTel("core_energy")
+	assert.Equal(t, metaUnit("core_energy"), unit)
+	assert.Equal(t, 1.0, scale)
+}