Skip to content
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

Fix Ingester Stutter #449

Merged
merged 8 commits into from
Jan 14, 2021
Merged

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Jan 11, 2021

What this PR does:
Ingesters currently show unreasonably high p99's on read and write. This is likely due to slow appends to the headblock. Note that under the hood it is inserting into a slice.

This PR reduces the time that we lock the active traces map by simply adding them to a linked list. Then we drop the active traces lock and only lock the blocks while we are adding them to the append block.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Member Author

joe-elliott commented Jan 11, 2021

Temporarily deploying this to our ops cluster is showing promising results on write:

image

Not seeing improvements to the read path indicating latency on reads is from another source. Perhaps we could use distributed tracing to research the read path 🤔.

@mdisibio
Copy link
Contributor

Great improvement. LGTM

modules/ingester/instance.go Outdated Show resolved Hide resolved
modules/ingester/instance.go Outdated Show resolved Hide resolved
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
tracesToCut := make([]*trace, 0, len(i.traces))

for key, trace := range i.traces {
if now.Add(cutoff).After(trace.lastAppend) || immediate {
if cutoffTime.After(trace.lastAppend) || immediate {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you move immediate to the front, it will short circuit the cutoffTime calculation. this may reduce the amount of time tracesMtx is held

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

immediate is almost never true. it only occurs when the ingester is shutting down to force flush traces from the active trace map to disk. i think this ordering is fine.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough, thanks for considering it.

tracesToCut := make([]*trace, 0, len(i.traces))

for key, trace := range i.traces {
if now.Add(cutoff).After(trace.lastAppend) || immediate {
if cutoffTime.After(trace.lastAppend) || immediate {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough, thanks for considering it.

Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@joe-elliott joe-elliott merged commit c974435 into grafana:master Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants