prometheus: expose descriptor metadata via Desc.Info - #2094
prometheus: expose descriptor metadata via Desc.Info#2094nicolastakashi wants to merge 3 commits into
Conversation
A Desc keeps its metadata unexported and offers only Err and String, so code that wants to know what a Collector declares has to parse the output of String. That format is not an API and has changed between releases. Add DescInfo and Desc.Info returning a structured, read-only view of the name, help, unit, variable label names and const labels. Also record the metric type on the Desc. The type is otherwise only observable through Gather, which skips any metric that has not produced a sample, so the type of a vector without children cannot be checked at all today. The typed constructors set it; a Desc built with NewDesc reports UNTYPED, which is accurate because a const metric carries its type per sample rather than on the descriptor. The metric type deliberately stays out of the id and dimHash calculations so that registration consistency is unchanged. Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
Registry already implements Describe, but draining a channel is awkward in a test. Add Registry.DescribeAll for a slice of the descriptors of every registered checked Collector, and testutil.CollectAndDescribe for the same over a single Collector. There is no GatherAndDescribe counterpart because the Gatherer interface does not expose descriptors. Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
| // for introspection, e.g. to check that a Collector describes the metrics a | ||
| // schema says it should. Info allocates on every call and is not meant for | ||
| // hot paths. | ||
| type DescInfo struct { |
There was a problem hiding this comment.
This needs more thoughts. We never exposed those info publicly. Apparently the reason was the need for changes in this structure. I wonder if this is still true 🤔
Some fact digged by AI:
Key Reasons Why Desc Fields Were Kept Private
1. Desc was viewed as an internal implementation detail, not a public user-facing abstraction
In PR #1309 (when a user asked to expose variable label names on Desc), @beorn7 summarized the history and design philosophy directly:
"The longer history here is that I was never really happy with
Desc. It felt like an implementation detail that the user should never see. Therefore, I've tried to keep it as much out of the way of the users as possible, and thought a lot about how to change the use cases where you still have to touch it in the future v2 version of this library.Adding exported methods to
Descwould go into the exact opposite direction.Another fundamental concern is that this library already has a lot of knobs for the users to turn. Adding more knobs requires strong justification, like simplifying a fairly common use case.
In this case, it feels we would help a relatively small number of users to simplify a relatively niche use case that could also be solved with a relatively simple work-around. All of that at the cost of increasing the cognitive load for the many many users of this library that do not need it."
2. Avoiding user dependencies on internal details that were slated for fundamental rework
Whenever users asked to export Desc fields or add getters, the maintainers held off because Desc was slated for a major redesign in Issue #222 ("Rework Desc and ConstMetrics") intended for a future v2:
-
In Issue #322 ("Expose Desc fields"), when
@AlekSirequested read access to fields likefqName:"The way Desc work will change fundamentally with Rework Desc and ConstMetrics #222. Tests in code using client_golang (in contrast to tests that test client_golang itself) must not depend on implementation details of client_golang. Simply making internal fields exported in Desc would only allow users to depend on all those internal implementation details (that are about to change anyway).
Right now, exposing Desc fields would be a step in the wrong direction."
-
In PR #326 ("Add getter methods for Desc() struct"), when getters were proposed:
"I'm reworking this for Rework Desc and ConstMetrics #222. I don't want to expose an internal structure in the main repo that is doomed to get changed in a couple of months."
-
In Issue #516 ("feature: get descriptor attributes functions"), when
GetName(),GetHelp(), andGetLabels()were proposed:"Descs are up for a major rehaul in the upcoming v0.10, see Rework Desc and ConstMetrics #222. I would prefer to not try to make the current Descs more sophisticated at this point, just to break everybody anyway in a few months. Depending on the exact new way Descs will be handled, accessors as you suggest might be something we could add, or they might become not needed (if, for example, a Desc is created by an Opts struct whose fields are exported anyway)."
3. Preserving strict immutability and thread safety
By contract, Desc represents the immutable metadata of a metric shared across collectors and goroutines during scrapes.
- Exporting fields directly would allow callers to mutate them (e.g., altering
variableLabelsslices or label values), violating the concurrency guarantees required duringCollect()andGather()operations. - In Issue #269,
@beorn7noted that modifying existing descriptors would "violate the contract about Descs being immutable and Write being concurrency-safe."
4. Enforcing registry invariants and preventing anti-patterns
Desc was introduced in 2014 (commit 5d40912f) to calculate precomputed hashes (id and dimHash) so the Registry could detect collisions and validate consistency (e.g. matching label dimensions and help text for identical metric names) at registration time rather than scrape time.
- Exposing
Descinternals to encourage dynamic label routing or building ad-hoc registries was viewed as encouraging anti-patterns (e.g., PR Add getter methods for Desc() struct #326 and PR Get variable label names from metric descriptors #1309).
What
Adds a way to ask a
Collectorwhat it declares, without parsingDesc.String().Desc.Info() DescInforeturns name, help, unit, variable label names, const labels, and the metric type.Descnow records the metric type. Typed constructors set it; aDescfromNewDescreportsUNTYPED.Registry.DescribeAll()returns the descriptors of every registered checked Collector.testutil.CollectAndDescribe()does the same for a single Collector.Purely additive. No exported symbol changes signature or behaviour.
Why
There is no structured way to read a descriptor. Anything that wants a metric's declared name, help, unit, or labels has to parse
Desc.String(). That format is not an API and it has changed: one release rendersvariableLabels: [a]with no unit, another rendersunit: "..."andvariableLabels: {a}. A test built on it breaks on aclient_golangbump.Gathercannot see a metric that has not produced a sample. ACounterVecwith no children reports nothing, so you cannot check its type, its labels, or its help. One Prometheus package declares 8 metrics, and 2 of them show up in a scrape of an idle instance. Anything checking metric compatibility against gathered output covers a quarter of that package.Describealready reaches all 8, but nothing could inspect a descriptor, and a descriptor could not say what type the metric was anyway, because the type lived only on the concrete metric. That is whyDescnow carries one.#2004 asks for this directly: compatibility tests for renamed metrics, removed metrics, and changed labels or types. The blocker in that thread is not being able to ask a
Registrywhat is registered. The breaking change it cites, FluentBit'sfluentbit_hot_reloaded_timeschanging from gauge to counter, is a type change.There is a working consumer at prometheus/prometheus#19523. It holds a package's metrics to a declared schema and catches renames, removals, added and removed labels, and type changes, including on metrics that never produce a sample.