proposal: Prometheus internal telemetry as an OTel semantic convention registry - #86
nicolastakashi wants to merge 7 commits into
Conversation
459e052 to
293d5de
Compare
|
I would like to assess what alternatives we have that could be built-in e.g. client_golang and would be lighter. And I would also want to better understand the scope of this: |
63633cc to
52b69ae
Compare
@roidelapluie thanks for comment, I've updated a few things in the doc aiming to cover what you raised, let me know your thoughts |
ArthurSens
left a comment
ArthurSens
left a comment
There was a problem hiding this comment.
Had some minutes in the airport to review this, it's not a complete review though 😅
…n registry Define every metric the Prometheus binary exports in one OTel semantic convention registry. Generate the instrumentation code and the docs from it, check the code against it with a Go test in CI, and publish it so downstream projects can check their own metric references. Contract testing reads Collector.Describe() rather than a scrape, because a running instance emits far less than it declares and a scrape cannot show units or the const-versus-variable label split. That path also catches a metric that was defined but never registered, which code generation cannot prevent. Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
9ab6b19 to
fbf87b5
Compare
Six changes to the semconv proposal, each checked against the tree. Scope. The document describes "every metric the Prometheus binary exports", which also covers go_*, process_* and promhttp_*, whose metadata we do not define -- go_* varies with the Go version and the WithGoCollectorRuntimeMetrics options at cmd/prometheus/main.go:379-388, and process_* is registered on Linux only -- and, read literally, descriptors defined here but never served: documentation/examples/remote_storage/remote_storage_adapter alone defines four. Add a rule keyed on descriptor definition plus server exposure, and narrow the TL;DR, the goals and the registry section to match. prometheus_build_info is the one case on the line; the rule excludes it and it stays excluded until we decide otherwise, which belongs in Non-Goals as an open decision rather than asserted either way. Descriptor type. The proposed accessors cover what Desc exposes today: name, help, unit, labels. Desc carries no type -- it lives on the concrete metric -- so instrument and annotations.prometheus.histogram_type have nothing to compare against, and a gauge becoming a counter is invisible to the only check there is. Now that in-process is the only path, this is load-bearing. Typed constructors can set it; NewDesc reports UNTYPED, so the three non-test call sites in scrape/metrics.go and discovery/file/file.go should convert. Units. prometheus.Opts has a Unit field and nothing in Prometheus sets it, so every descriptor reports empty while the example declared unit: s. Keep the registry's unit as metadata and require the descriptor's to stay empty, reporting non-empty as a difference, so an attempt to populate Opts.Unit is visible without blocking the first package. That edit is observable: Opts.Unit reaches the descriptor hash, MetricFamily.Unit, # UNIT in OpenMetrics, and from there scrape and TSDB metadata, remote write and /api/v1/metadata. The "Describe() returns the unit" claim is qualified accordingly. Labels. Label names are compared -- the text mentions the const-versus-variable split a scrape drops -- but nothing said how a label is declared and no example had one. Add an attribute_group with a namespaced ID, examples, and annotations.prometheus.label_name, referenced by ref, using prometheus_target_interval_length_seconds and its interval label (sl.interval.String(), hence "15s"). An attribute ID names a concept and a label name is a wire name: WAL record type and appended-sample type both carry label_name: type and are not the same thing. The ID conventions the examples use are stated as proposals to confirm. Package fixtures. This answers the "mapping registry entries to packages" open question rather than adding one, so that bullet goes -- but only because the field is now named and shaped: annotations.prometheus.package holds a repository-relative package directory, so it doubles as the <package>/internal/semconv/ output path generation needs. Both examples carry it. The comparison runs per package against the entries naming it, with table-driven fixtures where collectors depend on configuration. Also: stability: stable in the example contradicted "migrated metrics carry development"; brief carries a terminal period where Help does not (tsdb/compact.go:121-128), so annotations.prometheus.help holds the client_golang string verbatim and is the compared field; the summary example gains objectives mirroring scrape/metrics.go:199, since a Summary cannot otherwise be generated, and summary was the one histogram_type with no Rego rule behind it; the annotations inventory now lists what it actually carries; the .With() reviewer link 404s, r2736198866 is live; and deprecated is not replaced as a stability value but marked deprecated upstream, with a separate structured field carrying the reason and renamed_to. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Separate descriptor inventory checks from collected histogram representations and custom-collector types. Preserve dynamic collection semantics and require populated fixture coverage through production registration paths. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
proposal: clarify internal telemetry scope, contracts, and registry delivery
Follow-up to the merge of #1. Keeps every technical correction from that revision and edits it down. Stability, not maturity. The OTel semconv field is spelled stability; what changed upstream is that deprecated stopped being one of its values and moved into a structured field. Rename all three occurrences so the Goals bullet, the generation section and the lifecycle section agree with the schema. Alternatives. Both entries had been hedged into neutrality and no longer said which way we went. Restore an explicit verdict to each while keeping the corrections that prompted the hedging: live-check does accept JSON samples and does not require a Collector or a Rust toolchain, and generating the registry from code can carry lifecycle metadata provided something else supplies it. Those concessions are what the arguments now rest on. Ecosystem section. The families-to-series explanation had grown to three paragraphs and buried the claim the section exists to make. Cut to one paragraph and restore the closing scope sentence. Contract testing. Dropped the fixture matrix for empty, populated and removed jobs, and the zero-valued metadata detail. Both are implementation choices for the first package rather than decisions this proposal needs to settle. The histogram representation check, the UNTYPED custom-collector check and the coverage requirement stay. Prose. Removed em dashes, colons used as mid-sentence connectors, and a few sentences that stacked three clauses. Replaced line-number citations with package references so the document does not rot against the tree. Cut the prometheus_build_info case to two sentences and pinned the resolved output format without naming a Weaver CLI flag. Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
|
Thanks for putting this together, @nicolastakashi. I'm on board with the general direction. Generating metric definitions instead of hand-writing them is a good thing. That said, I want to echo @roidelapluie on lighter built-in alternatives and @ArthurSens on tying the solution to the goals. For metrics alone, I'm not sure OTel semconv is the right fit, since a lot of what actually matters to us ends up in The bigger point: if we pull Weaver in, we should standardize tracing and logging the same way. Metrics alone mostly buys us a bit more consistency, which I don't think is worth it on its own. All three signals would be. Do you want to scope traces and logs here, or is there a reason metrics need to stand alone first? |
Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
b0bce0b to
fd377a7
Compare
I understand the sentiment, but the main benefit is to actually reuse the tooling and standard. Notably the main goal was to integrate further with potential query normalization, etc, docs/tools that gather's what metrics each of their deployed component produces. Do we want (have cycles?) to reimplement simpler schema types and schema engine? |
| * [Required] One machine-readable registry describes every Prometheus-owned metric, in the sense fixed under Scope. The Go code stops being a second source of truth. | ||
| * [Required] No hand-written metric descriptors. Instrumentation code comes from the registry. | ||
| * [Required] Generated documentation, which therefore cannot drift. | ||
| * [Required] A CI check that catches drift between the registry and the binary, with no OTel Collector and no Weaver binary in the test path. |
There was a problem hiding this comment.
If this a must and "No hand-written metric descriptors. Instrumentation code comes from the registry." is a must, then I assume no weaver is allowed for generation either?
6 participants
This proposal defines all metrics exported by the Prometheus binary as a formal OTel semantic convention registry. Making the schema machine-readable enables auto-generated instrumentation code, always-in-sync documentation, contract testing against live instances, and a lifecycle model for safe metric evolution across the Prometheus ecosystem.
Proof-of-concept implementation: prometheus/prometheus#17868