docs: document exported metric names and coverage gaps - #2111
Conversation
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
27da803 to
5dc2e0a
Compare
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
📊 Token Validation BenchmarkComparison of this PR against the base branch. 🟢 improvement · 🔴 regression · ➖ within ±1.0% noise.
|
AkramBitar
left a comment
There was a problem hiding this comment.
Reviewed the docs against the code: the content is accurate. Every metric name, label value and scope claim matches production wiring today (traced the NewTMSProvider wrapper in token/core/{fabtoken,zkatdlog}/v1/driver/driver.go through ws.go into identity/role/idemix, and the DI-container providers for ttx / auditor / sherdlock / certifier / fabricx). All 15 files that declare metric opts are covered by the group list, the 54 table rows match the 54 golden lines, the monitoring.md URL change fixes a real 404, and go vet / gofmt / goimports / misspell / golangci-lint plus go test -count=3 -race -shuffle=on are clean.
The comments below are all about the new reference_test.go guard rather than the documentation. Two of them I'd suggest addressing before merge:
- The guard cannot detect the one change that would actually invalidate the whole page (the
scopefield is unverified metadata). - The unconditional histogram-suffix trim will fail on a correct page as soon as someone adds a metric whose name ends in
_count,_sumor_bucket.
The remaining four are polish — confusing failure output, a gap in the reverse check, a dead field, and a global that is only safe while the tests stay sequential.
|
Follow-up: bring back #1749 shipped a Grafana dashboard next to its metrics doc — https://github.com/LFDT-Panurus/panurus/pull/1749/files (permalink) — 17 panels over 9 rows, with That PR closed unmerged, and
Two things beyond the rename:
Do we want the dashboard in this PR, or as a follow-up? I have no strong preference. Adding it here keeps the dashboard and the names it depends on in one changeset, which is nice for review — but this PR is already sizeable and purely documentation, so a separate PR is equally reasonable. A follow-up would need to be based on this branch rather than |
5dc2e0a to
6a96c03
Compare
ccb3c91 to
83d60d7
Compare
Dismissing my own review: all six findings are fixed in 83d60d7 and each thread is answered and resolved.
Not converting this to an approval on purpose — I wrote most of the code now (the guard hardening and the Grafana dashboard), so approving it would be a self-review. This PR still wants a fresh pair of eyes from another maintainer.
|
@HayimShaul will be out of the office for the next two weeks. I'll be covering for him during that time, so I won't be able to continue reviewing this PR. Someone else will need to take over the review. |
83d60d7 to
4e4c4a0
Compare
4e4c4a0 to
e0c7c29
Compare
The SDK never spells out the metric names Prometheus exports. The fully-qualified name is assembled at registration time from the Go package of the caller, and the TMS-scoped provider in token/core/common/metrics adds a stack frame, so metrics created through it are exported under that package rather than their own. Documentation written from the bare Name field of the options therefore lists names no query matches. Add docs/development/metrics.md: the derivation rules, every metric under the name Prometheus actually serves, its type, labels, meaning and source, example queries, and a ranked analysis of what a node still cannot report. Guard the page with token/services/metricsdoc, which instantiates every metrics constructor the way production wires it, reads the resulting names back out of a Prometheus registry, and compares them against a golden file and against the page itself. Renaming, adding or relocating a metric fails the test until the documentation is updated. Which provider a constructor receives is as much a part of the exported name as the opts are, so it is checked rather than assumed. The token drivers are pinned as the only files in the repository that build a TMS-scoped provider, and as building it from the container's provider, so the scope of each group follows by exhaustion; dropping the wrapper fails the test instead of quietly renaming twenty-one metrics. Every production call site a group claims must also still contain that call. Two ways the guard could reject a correct page are fixed: the PromQL suffix fold now applies only onto registered histogram families, so a metric whose own name ends in _count, _sum or _bucket survives it; and the reverse check uses an explicit prose allowlist rather than accepting any documented name that is a prefix of a registered one, which had let truncations through. Each group is instantiated against its own provider and registry, so the declared and exported metrics cannot be mispaired by the provider's deduplication, and a genuine clash is reported by name. Add the Grafana overview dashboard, revived from #1749 with the names this change pins. That dashboard closed unmerged because every one of its 51 panel queries and 4 variable queries named the bare Name field from the Go source and so matched nothing. The substitution here is derived from the golden file rather than transcribed. A row is added for the four identity signer-resolution metrics that had no panel, so every exported metric is now covered. The dashboard's queries are guarded too. Each check covers a failure Grafana does not report as an error - it renders "No data", which is indistinguishable from an idle node: every metric named must be registered, every name must carry its package prefix, every label filtered or grouped on must be declared by that metric, and every variable must be a Grafana built-in or declared by the dashboard. Run against #1749's original file, the guard reports 53 findings. PromQL syntax is deliberately not parsed: it would need a new dependency, the exprs contain Grafana variables that are not valid PromQL, and it is the one failure mode Grafana surfaces itself. The guard checks names, not rendering; the file has not been imported into a live Grafana, so panel presentation is unverified. Export NewMetrics/NewClientMetrics in the auditor, certification and Fabric-X queue packages so the guard can build their instrumentation without constructing the surrounding service. Correct docs/drivers/metrics.md, which listed declared names as if they were queryable, and make docs/development/monitoring.md the entry point. Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com> Signed-off-by: AkramBitar <akram@il.ibm.com> Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
e0c7c29 to
086b1d4
Compare
Fixes #1745
Problem
Panurus exports 54 metrics and documented none of them under the name Prometheus actually serves.
Nothing in the SDK spells those names out. The fully-qualified name is assembled at registration time
from the Go package of the caller, and the TMS-scoped provider in
token/core/common/metricsadds astack frame — so metrics created through it are exported under that package, not their own. Docs
written from the bare
Namefield therefore list names no query matches. That is why #1749 was closed.What this PR does
1. Documents the exported names —
docs/development/metrics.md: how a name is derived, one tableper subsystem (exported name, type, labels, meaning, source), PromQL examples, and a ranked
## Coverage gapssection on what a node still cannot report.2. Guards the page —
token/services/metricsdocinstantiates all 16 metrics constructors the wayproduction wires them, reads the names back out of a Prometheus registry, and checks them against
testdata/metrics.goldenand against the page in both directions. It also pins the wiring the namesdepend on: the two token drivers are asserted to be the only places a TMS-scoped provider is built, so
dropping that wrapper fails the build instead of silently renaming 21 metrics.
3. Adds a Grafana dashboard —
docs/monitoring/grafana/token-sdk.json, revived from #1749 with thecorrect names, plus a row for the four
identity_*metrics that had no panel. All 54 metrics covered,19 panels over 9 rows. Its queries are guarded too: every metric must be registered, every name must
carry its package prefix, every label filtered or grouped on must be declared, every
$variablemustexist. These are the failures Grafana renders as "No data" rather than as an error.
4. Corrects
docs/drivers/metrics.md, which listed declared names as if they were queryable, andmakes
docs/development/monitoring.mdthe entry point.NewMetrics/NewClientMetricsare exported in the auditor, certification and Fabric-X queue packagesso the guard can build their instrumentation without constructing the surrounding service.
Verification
wiring entry, truncated name in the page, absent label, undeclared variable.
have failed the build.
make checksandmake lintexit 0; package green under-count=3 -race -shuffle=on.Known limitation
The dashboard guard checks names, not rendering. PromQL is not parsed (it would need a new
dependency, the exprs contain Grafana variables that are not valid PromQL, and it is the one failure
mode Grafana surfaces itself), and the file has not been imported into a live Grafana against a live
Prometheus — so panel presentation is unverified.
Follow-up
The metric-hygiene items in the gap analysis (package attribution, stuttering names, missing
network/channel/namespacelabels) all change exported names, so they belong in a singledeliberate follow-up with a release note rather than here.