fix(database_observability.postgres): Use of defer in explain_plans collector - #6819
Open
cristiangreco wants to merge 2 commits into
Open
fix(database_observability.postgres): Use of defer in explain_plans collector#6819cristiangreco wants to merge 2 commits into
explain_plans collector#6819cristiangreco wants to merge 2 commits into
Conversation
…tgres Fix `explain_plans` collector so that a cached query is finalised immediately instead of being deferred (which prevented correct update of processedCount and thus made batch-size limit ineffective).
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes database_observability.postgres explain plan collection so the batch-size limit is enforced correctly by finalizing each cached query immediately (instead of deferring cache cleanup / accounting until the end of fetchExplainPlans).
Changes:
- Replace per-iteration
defercleanup with immediate per-query finalization andprocessedCountincrement. - Extract per-query processing logic into
processExplainPlanand return a denylist decision to the caller.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/component/database_observability/postgres/collector/explain_plans.go:626
- This error log uses c.logger, which drops the per-query context (query_id/datname) already attached to the local logger. Logging via logger helps operational debugging when Loki output emission fails.
if err := c.sendExplainPlansOutput(
qi.datname,
qi.queryId,
generatedAt,
database_observability.ExplainProcessingResultSuccess,
"",
explainPlanOutput,
); err != nil {
c.logger.Error("failed to send explain plan output", "err", err)
}
internal/component/database_observability/postgres/collector/explain_plans.go:538
- This log line uses c.logger even though a request-scoped logger with query_id context is already available. Using logger here makes the error easier to correlate to the specific query being processed.
This issue also appears on line 617 of the same file.
if strings.HasSuffix(qi.queryText, "...") {
err := c.sendExplainPlansOutput(
qi.datname,
qi.queryId,
generatedAt,
database_observability.ExplainProcessingResultSkipped,
"query is truncated",
nil,
)
if err != nil {
c.logger.Error("failed to send truncated query skip explain plan output", "err", err)
}
cristiangreco
marked this pull request as ready for review
August 5, 2026 09:54
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.
Brief description of Pull Request
Fix
explain_planscollector so that a cached query is finalised immediately instead of being deferred (which prevented correct update of processedCount and thus made batch-size limit ineffective).Pull Request Details
Followup from #6804 (comment)
Issue(s) fixed by this Pull Request
n.a.
Notes to the Reviewer
PR Checklist