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

Search: Enforce Max Trace Size Bytes #1318

Merged
merged 5 commits into from
Mar 2, 2022

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Mar 2, 2022

What this PR does:
Uses the per tenant max trace size bytes to enforce limits on search. This has primarily been put in place to reduce serverless OOMs.

Once concern I have about this PR is that it adds a skippedTraces field to the returned search metrics. This field is named similarly to skippedBlocks but they have very different meanings.

  • Block skipping occurs in the ingester and indicates that an entire block didn't need to evaluated for the search conditions. It is a performance improvement.
  • Trace skipping occurs in the queriers/serverless functions and indicates a trace not evaluated for the conditions provided. It indicates that there is potentially a trace missing from your results.

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>
@zalegrala
Copy link
Contributor

This looks reasonable to me, though I think perhaps some documentation about the expected behavior would be good. I'm not sure who would expect to be searching for large traces, but if they were, knowing what to expect up front seems prudent.

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

This looks reasonable to me, though I think perhaps some documentation about the expected behavior would be good. I'm not sure who would expect to be searching for large traces, but if they were, knowing what to expect up front seems prudent.

Yeah, I agree. I need to generally improve the search/serverless docs.

@mdisibio
Copy link
Contributor

mdisibio commented Mar 2, 2022

LGTM - could we update the comments in limits.go for MaxBytesPerTrace?

Signed-off-by: Joe Elliott <number101010@gmail.com>
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.

None yet

3 participants