Skip to content

Reject repeated Enum states - #1240

Open
chrikrah wants to merge 1 commit into
prometheus:masterfrom
chrikrah:fix/enum-duplicate-states
Open

chrikrah wants to merge 1 commit into
prometheus:masterfrom
chrikrah:fix/enum-duplicate-states

Conversation

@chrikrah

Copy link
Copy Markdown
Contributor

Enum accepts a states list with a repeated entry and exposes one sample per entry, so the family carries two e{e="a"} samples. After state('a') they disagree, and the OpenMetrics parser keeps only the first.

# base ff9a2d3, python 3.12; repro.py builds Enum('e', 'h', states=['a', 'b', 'a']), calls
# state('a'), prints the OpenMetrics exposition and parses it back
$ python repro.py
# HELP e h
# TYPE e stateset
e{e="a"} 1.0
e{e="b"} 0.0
e{e="a"} 0.0
# EOF
parsed: [({'e': 'a'}, 1.0), ({'e': 'b'}, 0.0)]
$ python repro.py 2>&1 | tail -1   # this branch
ValueError: Duplicate states for Enum metric e: ['a']

That breaks the OpenMetrics StateSet rule, "StateSets MUST have one sample per State", and enum.md, which promises one time series per state. The check sits with the other Enum argument checks, ahead of super().__init__(), so a rejected metric is never registered, as #1189 arranged for empty states.

Verification

$ python -m pytest -q
429 passed, 14 skipped, 28 subtests passed   # this branch
428 passed, 14 skipped, 28 subtests passed   # base ff9a2d3
$ python -m pytest -q   # metrics.py reverted, test kept
1 failed, 428 passed, 14 skipped, 28 subtests passed   # TestEnum::test_duplicate_states
$ flake8 prometheus_client/ tests/   # flake8 7.4.1
$ isort --check prometheus_client/ tests/   # isort 5.10.1
$ mypy prometheus_client tests/test_core.py   # mypy 2.4.0
Found 5 errors in 2 files   # identical at base
# not run: tox on other interpreters, PyPy

There is no issue for this. #1234 rewrites the same lines of Enum.__init__ to accept an enum class, whose member names cannot repeat, so whichever lands second needs a one-line rebase.

@csmarchbanks, this raises for the reason you gave on #1214: a repeated state is almost certainly a typo. A caller who has one today gets a ValueError at construction in place of a contradictory sample. Happy to land it before or after #1234, whichever you merge first.

Enum(states=['a', 'b', 'a']) exposes e{e="a"} twice in one family. After
state('a') the two samples disagree, 1.0 and 0.0, and the OpenMetrics
parser keeps only the first. Raise at construction, before registration,
as duplicate label names already do.

Signed-off-by: Christopher Krah <48338417+chrikrah@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant