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

Ingester prevent writes to large traces even after flushing #1199

Merged
merged 5 commits into from
Jan 5, 2022

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Jan 4, 2022

What this PR does:
This PR updates the ingester to keep track of traces that exceeded max size and reject further writes to them, even after they are flushed to disk. The list of traces is reset when cutting the head block.

Which issue(s) this PR fixes:
Fixes part of #1133

Checklist

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

…ing to disk, keep rejecting writes until reset when cutting block
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

minor thoughts. lgtm

CHANGELOG.md Outdated Show resolved Hide resolved
modules/ingester/instance.go Show resolved Hide resolved
modules/ingester/trace.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -32,6 +32,7 @@
* [BUGFIX] Ensure that the admin client jsonnet has correct S3 bucket property. (@hedss)
* [BUGFIX] Publish tenant index age correctly for tenant index writers. [#1146](https://github.com/grafana/tempo/pull/1146) (@joe-elliott)
* [BUGFIX] Ingester startup panic `slice bounds out of range` [#1195](https://github.com/grafana/tempo/issues/1195) (@mdisibio)
* [BUGFIX] Prevent writes to large traces even after flushing to disk [#1199](https://github.com/grafana/tempo/pull/1199) (@mdisibio)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think another solution that might cover more cases is returning an error from CombineTraces() as soon as the pieces sum up to > maxbytes. That will help fix compaction as well as querying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, we can explore that in a separate PR. Let's continue collecting thoughts in #1133 and #976.

@mdisibio mdisibio merged commit 02dab0c into grafana:main Jan 5, 2022
@mdisibio mdisibio deleted the ingester-large-trace-tracking branch January 6, 2022 17:36
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.

3 participants