Repository navigation
Say the count of a coalesced event is in its tag, not its value - #43
Merged
Merged
Conversation
The documented formula summed `value` for events tagged `coalesced`, which is only right for an event reported without a value: one reported with a value keeps it, and only the tag holds the count. The README's own example reports `command` with 1.0, and its text called `command` valueless. Also say what the code already does: a count that finds the queue full is lost (pinned by a new test), a report after close() is dropped, install is left off at 32 tags, a comment at column 0 does not end a tags: block, the server-wide switch takes the same spellings as isOff, and installIdFromFile writes nothing for a null or blank path. No production code changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Self-review rubric (posted as a comment; this is not an independent review):
Repo-specific:
Findings:
Do-not-auto-merge check: no listed path matches. The README "Opting out" and "Keys" sections, This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener). drafted by Claude on behalf of Daniel Stephenson |
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.
Summary
Stage A (documentation accuracy sweep) cycle. Every README section and the Javadoc were checked against the code. No production code is changed: the
TraceClient.javadiff is comments only.Wrong claim corrected
valuefor events taggedcoalesced. That only holds for an event reported without a value. An event reported with a value keeps it (flushsendswindow.valuewhen it is not null), sovalueis not the count and only thecoalescedtag holds it.coalesce_keysOnNameValueAndTagsButNotTagOrderalready pins this:value2.5 withcoalesced2. The README also calledcommandvalueless while its own example reportscommandwith1.0, so a query following the old text would undercount that example. The README and thereportJavadoc now give the tag as the count, andCOALESCE(value, 1)is kept as the shortcut for valueless events.Claims made more exact
enqueuereturns without queueing it, andflushhas already reset or removed the window). The README saidclose()sends every count still open. This was not pinned by any test, soclose_aCountThatFindsTheQueueFullIsDroppedis added.close()is dropped. Already pinned byreport_afterCloseIsDroppedWithoutThrowing.installtag is left off an event that already holds 32 tags withversion. The wire format section saidinstallwas present whenever the client has an ID. Already pinned bywithInstall_addsTheIdOnlyWhenThereIsOneAndRoomAndNoneAlready.tags:block. The README and theparseServerWideConfigJavadoc said the block ends at the next line that is not indented. Already pinned byserverWideTags_skipBlankAndCommentLinesInsideTheBlock.off,0andnoin any case, as well asfalse. Stated in the class andBuilder.serverWideConfigJavadoc. Already pinned byserverWideConfig_acceptsEverySpellingOfOff.installIdFromFilewrites nothing for anullfile or a blank path. Already pinned byinstallIdFromFile_neverThrowsForABadPath.Merge gate
The README "Opting out" and "Keys" sections,
pom.xml, the workflows, the version strings and every wire-format or opt-out constant are untouched, so no do-not-auto-merge path matches. The README row for the spelling fix above is in the protected "Opting out" section, so it was left out and filed as #42.Triage notes (skipped issues)
No issue is closed by this PR. #13, #15, #27, #31, #39 and #40 each ask for a maintainer decision on the wire format or the opt-out contract, and each lists options, so none was picked up for autonomous implementation. #42 is in the same position. The body of #31 quotes the old formula. Its substance still holds, because a count sent without the tag is lost under either formula.
Test plan
mvn -B verifyrun locally on Java 21:Tests run: 107, Failures: 0, Errors: 0, Skipped: 0(106 before this change)enqueuetemporarily raised by one, it went red with the count in the delivered bodies, and the file was restored afterwardsCI does not cover anything host-specific here, and nothing in this PR depends on a real server.
No tracking issue: these gaps were found during triage.
This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).
🤖 Generated with Claude Code
drafted by Claude on behalf of Daniel Stephenson