-
Notifications
You must be signed in to change notification settings - Fork 436
database_observability: cache pg sample and wait events before sending finalised entries #4523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fec512f
to
5e19ebf
Compare
…vents for easier matching
…ype" This reverts commit d908305.
internal/component/database_observability/postgres/collector/query_samples.go
Outdated
Show resolved
Hide resolved
internal/component/database_observability/postgres/collector/query_samples.go
Outdated
Show resolved
Hide resolved
internal/component/database_observability/postgres/collector/query_samples.go
Outdated
Show resolved
Hide resolved
|
||
// emitAndDeleteSample builds final entries for a sample and removes it from memory. | ||
func (c *QuerySamples) emitAndDeleteSample(key SampleKey, state *SampleState) { | ||
// Build and emit OP_QUERY_SAMPLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like a lot of the godoc and inline comments are superflous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like documenting the functions behavior, it makes the code easier to read. I can remove it if you are strongly against though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a balance, a lot of the comments are saying what the code is doing. Their usefulness comes when they explain why it is doing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed those that I judged redundant. Let me know if it looks better.
internal/component/database_observability/postgres/collector/query_samples.go
Outdated
Show resolved
Hide resolved
} | ||
defer rows.Close() | ||
|
||
activeKeys := map[SampleKey]struct{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth having a single map keys = map[SampleKey]SampleState
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are separated to make finalize logic straightforward, differentiating between samples that turned idle from samples that disappeared. If we put it all together more logic will be needed to classify samples within the same map.
PR Description
Rework the Postgres query samples collector to buffer per-query execution state across scrapes, consolidate wait events, and emit finalized entries.
Key changes:
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist