om2: MetricFamilyName is MetricName #2791
Conversation
Fixes prometheus/OpenMetrics#305 Signed-off-by: bwplotka <[email protected]>
|
I am bit surprised but I couldn't find any special rule for those suffixes in the ABNF to change... also tests passes despite me adding suffixes to MF names. Something smells like ABNF is too relaxed. |
ArthurSens
left a comment
ArthurSens
left a comment
There was a problem hiding this comment.
I could review only half of the PR 😬, I have some small comments
| * MUST be the same as every MetricPoint's MetricName in the family. TODO(bwplotka): This assumes complex values will be introduced. Otherwise, we need to keep suffixes section for histograms and summaries. | ||
|
|
||
| ###### Suffixes | ||
| > NOTE: [OpenMetrics 1.0](https://prometheus.io/docs/specs/om/open_metrics_spec/#suffixes) was allowing different MetricFamily Name vs MetricName in the same family. For example MetricFamily Name `foo` for a counter with a MetricName `foo_total`. This is no longer accepted thanks to ComplexValue and for parser reliability and future compatibility. |
There was a problem hiding this comment.
| > NOTE: [OpenMetrics 1.0](https://prometheus.io/docs/specs/om/open_metrics_spec/#suffixes) was allowing different MetricFamily Name vs MetricName in the same family. For example MetricFamily Name `foo` for a counter with a MetricName `foo_total`. This is no longer accepted thanks to ComplexValue and for parser reliability and future compatibility. | |
| > NOTE: [OpenMetrics 1.0](https://prometheus.io/docs/specs/om/open_metrics_spec/#suffixes) allowed different MetricFamily Name vs MetricName in the same family. For example, MetricFamily Name `foo` for a counter with a MetricName `foo_total`. This is no longer accepted for parser reliability and future compatibility. |
I'm not sure it's needed to mention what thing enabled this to happen 🤔
There was a problem hiding this comment.
It many cases it required different MetricFamily Name vs MetricName.
| > NOTE: [OpenMetrics 1.0](https://prometheus.io/docs/specs/om/open_metrics_spec/#suffixes) was allowing different MetricFamily Name vs MetricName in the same family. For example MetricFamily Name `foo` for a counter with a MetricName `foo_total`. This is no longer accepted thanks to ComplexValue and for parser reliability and future compatibility. | ||
| The name of a MetricFamily MUST NOT result in a potential clash for sample metric names as per the ABNF with another MetricFamily in the Text Format within a MetricSet. An example would be a gauge called "foo_total" as a counter called "foo" could create a "foo_total" in the text format. | ||
| Names SHOULD be in snake_case. Names SHOULD follow the restrictions in the ABNF section under `metricname`. Metric names MAY be any quoted and escaped UTF-8 string as described in the ABNF section. Be aware that exposing UTF-8 metrics is still experimental and may reduce usability, especially when `_total` or unit suffixes are not included. |
There was a problem hiding this comment.
| Names SHOULD be in snake_case. Names SHOULD follow the restrictions in the ABNF section under `metricname`. Metric names MAY be any quoted and escaped UTF-8 string as described in the ABNF section. Be aware that exposing UTF-8 metrics is still experimental and may reduce usability, especially when `_total` or unit suffixes are not included. | |
| Names SHOULD be in snake_case. Names SHOULD follow the restrictions in the ABNF section under `metricname`. Metric names MAY be any quoted and escaped UTF-8 string as described in the ABNF section. |
Debatable if this should be removed at all, and definitely not in this PR... but isn't it weird that OpenMetrics mentions experimental features in Prometheus?
| The name of a MetricFamily MUST NOT result in a potential clash for sample metric names as per the ABNF with another MetricFamily in the Text Format within a MetricSet. An example would be a gauge called "foo_total" as a counter called "foo" could create a "foo_total" in the text format. | ||
| Names SHOULD be in snake_case. Names SHOULD follow the restrictions in the ABNF section under `metricname`. Metric names MAY be any quoted and escaped UTF-8 string as described in the ABNF section. Be aware that exposing UTF-8 metrics is still experimental and may reduce usability, especially when `_total` or unit suffixes are not included. | ||
|
|
||
| Colons in MetricFamily names are RESERVED to signal that the MetricFamily is the result of a calculation or aggregation of a general purpose monitoring system. |
There was a problem hiding this comment.
I don't know if this holds any value in the spec. When we mention 'RESERVED', are we instructing SDKs not to allow colons?
There was a problem hiding this comment.
I also don't know what RESERVED means? I don't think it is one of the keywords (like MUST, SHOULD, MUST NOT).
There was a problem hiding this comment.
I think we can say "MUST NOT be used unless ..."
| A Counter MetricPoint SHOULD have a Timestamp value called Start Timestamp. This can help ingestors discern between new metrics and long-running ones it did not see before. | ||
|
|
||
| A MetricPoint in a Metric's Counter's Total MAY have an exemplar. | ||
| A Counter MetricPoint MAY reset to 0. If present, the corresponding Start Timestamp MUST also be set to the timestamp of the reset. |
There was a problem hiding this comment.
| A Counter MetricPoint MAY reset to 0. If present, the corresponding Start Timestamp MUST also be set to the timestamp of the reset. | |
| A Counter MetricPoint MAY reset to 0. If so, the corresponding Start Timestamp MUST also be set to the timestamp of the reset. |
| MetricFamily names beginning with underscores are RESERVED and MUST NOT be used unless specified by this standard. | ||
|
|
||
| * Suffixes for the respective types are: | ||
| * Counter: `_total` |
There was a problem hiding this comment.
Feedback: on storage clash can happen (STORAGE classic clash)
# TYPE foo_count gauge
foo_count ...
# TYPE foo summary
foo { .....}
In storage you have
- foo_count
- foo_count classic representation
Unrelated: This is not possible in OM 2.0 (TODO double check if it's clear). (MetricFamily Type and unit uniqueness)
# TYPE foo counter
foo ...
# TYPE foo gauge
foo{ .....}
There was a problem hiding this comment.
Consensus: We add section around suffix clashes.
Q: SHOULD avoid collisions vs MUST avoid collision
Proposal: Keep MUST NOT for now?
| If no TYPE is exposed, the MetricFamily MUST be of type Unknown. | ||
|
|
||
| If a unit is specified it MUST be provided in a UNIT metadata line. In addition, an underscore and the unit SHOULD be the suffix of the MetricFamily name. | ||
| If a unit is specified it MUST be provided in a UNIT metadata line. In addition, an underscore and the unit SHOULD be part of the suffix chain of the MetricFamily name. |
There was a problem hiding this comment.
Just mentioning it's not a clear suffix anymore (it's not the last trailing part).
There was a problem hiding this comment.
TODO: Link to prometheus convention??
| without `_total` suffix may reduce the usability due to confusion about what the metric's type is. | ||
|
|
||
| Be aware that exposing metrics without `_total` being a suffix of the MetricFamily name directly to end-users may reduce the usability due to confusion about what the metric's type is. | ||
| // TODO Shouldn't it be before t@? |
| ##### Info | ||
|
|
||
| The Sample MetricName for the value of a MetricPoint for a MetricFamily of type Info MUST have the suffix `_info`. The Sample value MUST always be 1. | ||
| The Info MetricPoint's MetricName MUST have the suffix `_info`. // TODO(bwplotka): Shouldn't this "SHOULD"? |
There was a problem hiding this comment.
Consensus: Info MUST is fine for Otel. (no conflicting rule, no native info type). If we encode we can follow Prometheus convention.
| The Info MetricPoint's MetricName MUST have the suffix `_info`. // TODO(bwplotka): Shouldn't this "SHOULD"? | |
| The Info MetricPoint's MetricName MUST have the suffix `_info`. |
| > NOTE: [OpenMetrics 1.0](https://prometheus.io/docs/specs/om/open_metrics_spec/#suffixes) was allowing different MetricFamily Name vs MetricName in the same family. For example MetricFamily Name `foo` for a counter with a MetricName `foo_total`. This is no longer accepted thanks to ComplexValue and for parser reliability and future compatibility. | ||
| The name of a MetricFamily MUST NOT result in a potential clash for sample metric names as per the ABNF with another MetricFamily in the Text Format within a MetricSet. An example would be a gauge called "foo_total" as a counter called "foo" could create a "foo_total" in the text format. | ||
| Names SHOULD be in snake_case. Names SHOULD follow the restrictions in the ABNF section under `metricname`. Metric names MAY be any quoted and escaped UTF-8 string as described in the ABNF section. Be aware that exposing UTF-8 metrics is still experimental and may reduce usability, especially when `_total` or unit suffixes are not included. |
There was a problem hiding this comment.
I know this is just moved, but we should clarify that exposing UTF-8 metrics is still experimental doesn't apply to its support in this protocol, just to its support elsewhere in the Prometheus ecosystem.
Fixes prometheus/OpenMetrics#305
We can either merge this with TODOs before complex values/types or wait for complex type/values.