sql: limit statistics discard log message #110805
Merged
Conversation
This file contains 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
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
Problem: The discard log message occurs for every transaction end after the limit is hit. This causes the log to be flooded with discard messages. This is not useful for users and can cause issues with telemetry pipelines. Solution: The discard message will only be logged once per minute. The log rate is controlled by a cluster setting. This allows the message to be set to a very large message if this expected behavior for a cluster. Refactored: The SQLStats creates and hold the reference to the counts. Then each container which is per an app name is passed the counts by reference. It's not obvious that the counts are shared between the containers. The code was refactored to make a single object to hold the counts and pass all the related content together. This makes the code easier to read and expand in the future if other values need to be added. Fixes: #110454 Release note (sql change): The discard log message is now limited to once per minute by default. The message was also changed to have both the number of transactions and the number of statements that were discarded.
maryliag
approved these changes
Sep 20, 2023
There was a problem hiding this comment.
really nice refactor!
Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @j82w)
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 91af030 to blathers/backport-release-23.1-110805: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
j82w
added
release-blocker
craig bot
pushed a commit
that referenced
this pull request
Oct 3, 2023
111613: sqlstats: fix counter for in-memory fingerprints r=j82w a=j82w Problem: The counters used to track the number of unique fingerprints we store in-memory for sql stats were refactored in #110805. In change #110805 a bug was introduced where it incresease the memory instead of resetting the counts. This causes the statstics to stop calculating new stats once the limit is hit. Solution: Fix the bug by resetting the counters instead of increasing them. Added new test to test the reset functionality. Fixes: #111583 Release note (sql change): Fix a bug that causes the sql stats to stop collecting new stats. Co-authored-by: j82w <jwilley@cockroachlabs.com>
blathers-crl bot
pushed a commit
that referenced
this pull request
Oct 3, 2023
Problem: The counters used to track the number of unique fingerprints we store in-memory for sql stats were refactored in #110805. In change #110805 a bug was introduced where it incresease the memory instead of resetting the counts. This causes the statstics to stop calculating new stats once the limit is hit. Solution: Fix the bug by resetting the counters instead of increasing them. Added new test to test the reset functionality. Fixes: #111583 Release note (sql change): Fix a bug that causes the sql stats to stop collecting new stats.
THardy98
pushed a commit
to THardy98/cockroach
that referenced
this pull request
Oct 6, 2023
Problem: The counters used to track the number of unique fingerprints we store in-memory for sql stats were refactored in cockroachdb#110805. In change cockroachdb#110805 a bug was introduced where it incresease the memory instead of resetting the counts. This causes the statstics to stop calculating new stats once the limit is hit. Solution: Fix the bug by resetting the counters instead of increasing them. Added new test to test the reset functionality. Fixes: cockroachdb#111583 Release note (sql change): Fix a bug that causes the sql stats to stop collecting new stats.
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.
Problem:
The discard log message occurs for every transaction end after the limit is hit. This causes the log to be flooded with discard messages. This is not useful for users and can cause issues with telemetry pipelines.
Solution:
The discard message will only be logged once per minute. The log rate is controlled by a cluster setting. This allows the message to be set to a very large interval if this expected behavior for a cluster.
Refactored:
The SQLStats creates and hold the reference to the counts. Then each container which is per an app name is passed the counts by reference. It's not obvious that the counts are shared between the containers. The code was refactored to make a single object to hold the counts and pass all the related content together. This makes the code easier to read and expand in the future if other values need to be added.
Fixes: #110454
Release note (sql change): The discard log message is now limited to once per minute by default. The message was also changed to have both the number of transactions and the number of statements that were discarded.