[dbm] add optional execution_indicator to rule out false positive query metrics move #20037
+145
−3
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
datadog_checks_base/datadog_checks/base/utils/db/statement_metrics.py
Outdated
Show resolved
Hide resolved
datadog_checks_base/datadog_checks/base/utils/db/statement_metrics.py
Outdated
Show resolved
Hide resolved
| # 3. Execution indicators: If execution_indicators is specified, only consider a query as changed if at | ||
| # least one of the execution indicator metrics has changed. This helps filter out cases where an old or | ||
| # less frequently executed normalized query was evicted due to the stats table being full, and then | ||
| # re-inserted to the stats table with a small call count and slight duration change. In this case, |
There was a problem hiding this comment.
Can we detect the case when the query was re-inserted as opposed to the case where the metrics increase, but executions did not?
There was a problem hiding this comment.
that was already taken care of. any negative metrics diff are discarded.
There was a problem hiding this comment.
amw-zero
reviewed
Apr 9, 2025
| # If execution_indicators is specified, check if any of the execution indicator metrics have changed | ||
| if execution_indicators: | ||
| indicator_columns = execution_indicators & metric_columns | ||
| if not any(diffed_row[k] > 0 for k in indicator_columns): |
There was a problem hiding this comment.
I don't think this fully solves the problem. Example execution order:
queryid: -12345
count: 1
duration: 1500
-->
query eviction for -12345 occurs
-->
queryid: -12345
count: 3
duration: 4000
In this case, the count diff is greater than 0 even though an eviction has occurred, so the metric will be reported, but it will have incorrect values.
There was a problem hiding this comment.
we chat about this offline. when the count diff is greater than 0 after a normalized query eviction and re-insertion, we have no reliable to tell if a previous eviction is happened. in this case, the metrics for this normalized will be incorrect (mostly inflation) for the first check run after eviction.
nenadnoveljic
approved these changes
Apr 10, 2025
dkirov-dd
approved these changes
Apr 14, 2025
temporal-github-worker-1
bot
added
agent/approved
and removed
agent/review-requested
labels
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.
What does this PR do?
This PR added execution indicators to
StatementMetrics.execution_indicatorsparameter toStatementMetrics.compute_derivative_rowsexecution_indicatorsis specified, only consider a query as executed if at least one of the specified metrics has changed.execution_indicatorsbehaves like the original implementationFor example, in PostgreSQL we can use 'calls' as the execution indicator
Motivation
https://datadoghq.atlassian.net/browse/DBMON-4170
In database query metrics monitoring, we've observed cases where duration metrics (like
total_time) change slightly between check runs while execution counts remain the same. These small changes are often due to the prior old or less frequently used normalized query being evicted from the stats table (i.e. pg_stat_statements) then re-inserted with the same call count (usually 1) and slightly different duration. In this case, the newly inserted normalized query should be treated as the baseline metric for future diffs.Below is an example of a normalized query being evicted and re-inserted.
The query has same
queryid:-6478076666730767487because they are structurally the same. We can tell it's an eviction + re-insertion by the differenttraceparentvalue in the comment. When this happens, we will see a call count = 0 but slightly different execution duration change due to the diff between1015.0188ms (after) and1014.6086999999999ms (before).Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged