test: pin label vocabularies with laravel-telemetry 2.4 metric inspection - #2
Merged
Merged
Conversation
…tion
Membership assertions ("a series with label X exists") pass on a single
branch of an attribution, which is the wrong question wherever the label
value is the thing under test. Replace them — and the hand-rolled
collect() loops — with recordedMetrics() and exact label-set assertions:
- FrontendRequestTest: http.route on the request histogram is now pinned
to exactly the logical content route / not_found, one series.
- ListenersTest: glide ad-hoc manipulations must collapse into one
"custom" bucket (three distinct param sets, two series); the auth
vocabulary is the explicit map, nothing else.
- PublishStateTest: a new test saves all four publish states in one run
and pins the observed action set, covering `expired` — which the
status snapshot supports but no single-state test exercised.
- StaticCacheTest: miss/write and invalidate/flush are exact sets, so a
purge path counted twice under the wrong outcome fails.
- MetricsProviderTest: the entries gauge is one series per collection,
not just whatever samples[0] happens to be.
- CacheKeyGroupsTest (new): drives a Stache-scale keyspace through the
real cache counter, proving Hooks::register installs the classifier at
all and that ~70 raw keys stay under a 25-series budget with nothing
dropped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The overlay's counters attribute across bounded vocabularies — publish states, Stache key groups, static-cache outcomes, Glide presets, auth events. The tests asserted membership: "a series with
action=publishedexists". Every branch produces a valid value, so those assertions pass on one branch and stay silent about the other four. Several tests also hand-rolled a loop overcollect(), because the fake offered nothing better.telemetry 2.4's
recordedMetrics()does, so the tests now pin the full observed set.What each one can catch that it could not before
FrontendRequestTest— twoforeach ($fake->collect() …)loops withtoContain/not->toContainbecomeassertLabelValues('http.route', [...])->assertSeriesCount(1). Previously a third route value, or the/{segments?}catch-all surviving beside the logical route, passed.PublishStateTest— new test; the five per-state tests are untouched. Saves all four states in one run and pinsactionto exactly[published, draft, scheduled, expired]. A per-state test cannot see a status resolved once and reused across saves — all four collapsing onto one action still passes every one of them. It also coversexpired, whichentrySaveAction()supports but nothing exercised.ListenersTest— the Glide preset test now fires three different ad-hoc param sets and pinspresetto[thumbnail, custom]with a series count. That is the cardinality leak the listener's own comment warns about, and nothing tested it. Auth events: three membership assertions → one exact set, so a fourth value (an unmapped event falling back to a class name) fails.StaticCacheTest— miss/write and invalidate/flush are exact sets with a count, so double-counting under a wrong outcome is visible.MetricsProviderTest—collect(...)->first(...)+samples[0]read only the first sample; a duplicatepagesseries or a stray collection passed silently while doubling the dashboard total.CacheKeyGroupsTest(new) — drives ~70 realistic Stache keys through the realcache.operationscounter and asserts the exact six-bucket vocabulary plusassertCardinalityBelow(25)/assertLabelCardinalityBelow('key_group', 10), and thattotal()equals 3 × the key count so nothing is dropped. Boundedness is the property the classifier exists for; the unit test could not express it, and nothing covered the fact thatHooks::registeractually installs the classifier.tests/Unit/CacheKeysTest.phpis deliberately untouched — already exact per key, and rewriting it into a cardinality test would trade precise coverage for nothing.Verified, not assumed
Three deliberate breakages, each reverted. The one worth quoting: dropping
'expired'fromRecordContentChange::entrySaveAction()leaves all five pre-existing publish-state tests green and fails only the new one:Removing
classifyCacheKeysUsing()fromHooks::registeris invisible to the unit test and failsCacheKeyGroupsTest. Making the Glide preset fall back to raw param keys fails withunexpected: fit, w, w,h.Tests only —
git diff main -- src/is empty. pint, PHPStan level 9 and 79 tests all green.Noticed, not touched
src/StaticCaching/StaticCacheTelemetry.php:91says "count each outcome once per request", but the guard is a single last-value slot:$request->attributes->get(RECORDED_KEY) === $result. It suppresses immediate repeats only, so amiss → write → misssequence in one request — which the comment itself says happens, via middleware plus nocache — countsmisstwice. Either the comment overstates the guarantee or the slot should be a per-outcome set. No test here encodes the current behaviour as correct.