fix(metrics): count a command under the label actually typed - #72
Merged
Conversation
Every invocation incremented both the command family under the canonical name and the alias family under the typed label, so one /sethome reported as a use of create-home as well. That made the alias charts useless for the thing they exist for, deciding which labels to retire, because a canonical chart could not be told apart from alias traffic. An invocation now increments exactly one family: the canonical name goes to command, anything else to alias. commands_total sums both families so it keeps meaning every invocation whatever label was typed. Expect a step down in the command_* lines on the dashboard where alias traffic used to be folded in. Chart ids are unchanged.
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
1 participant
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 bug
CommandUsageListenerincremented two counters on every invocation, the COMMAND family under the canonical name and the ALIAS family under the typed label. One/sethometherefore reported as a use ofcreate-homeas well as a use ofsethome.That defeats the reason the alias charts exist. The point of counting commands is to decide which commands and aliases can be retired, and
command_create_homecould not be told apart from alias traffic folded into it.The change
An invocation now increments exactly one family, chosen by the label actually typed. The canonical name goes to COMMAND, anything else to ALIAS. The namespaced form has its prefix stripped first, so
/sethomestwo:sethomestill lands with the plain alias.WindowShare.totaltakes varargs, andcommands_totalis nowtotal(COMMAND, ALIAS)so it keeps meaning every invocation whatever label was typed. Putting the sum there rather than in the chart lambda keeps it testable, sinceBStatsHandlecannot be constructed under MockBukkit.Chart ids are unchanged, so nothing needs re-registering on bstats.org.
Tests
Three existing tests encoded the old behavior and were inverted before the fix, so all four failures were observed red first. The new
aCommandAndItsAliasAreCountedSeparatelytypes/create-homeonce and/sethometwice; against the old code it failed withexpected: <1> but was: <3>, and now reportscommand_create_home = 1,alias_sethome = 2. A newWindowShareTestcase covers the multi-family total and asserts each family is still drained only once per submission.mvn verify: 389 tests, 0 failures, 0 skipped.Note for the dashboard
Expect a step down in the
command_*lines where alias traffic used to be folded in. The metrics shipped in 2.0.0, so there is existing history under the old semantics.