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

Loki: Change live tailing to only allow mutating the log line not the number of streams. #6063

Merged
merged 5 commits into from
May 2, 2022

Conversation

slim-bean
Copy link
Collaborator

@slim-bean slim-bean commented May 1, 2022

What this PR does / why we need it:

Stops live tailing from splitting a stream into more streams when using parsers while still maintaining the ability to mutate the log line using parsers and line_format

The current implementation will allow creating many new streams from a tailed stream when using parsers, but this multiplicative affect can make it difficult to then tail the stream because it creates a lot more overhead in the amount of data pushed down the websocket.

For the purposes of live tailing which is meant primarily for human consumption, returning the parsed and extracted labels and manipulating the streams isn't helpful for the end user visualization and instead makes the process less efficient and more likely to struggle to keep up with higher volume streams.

Update: Also included in this PR is a change to no longer reuse the same pipeline for parsing every push request, the pipeline has a label cache built into it which, when reused, would lead to unbounded memory growth when the log lines being processed had extremely high cardinality data being parsed.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

…e with the pipeline to allow mutating the content of the line but don't split it into new streams.

Signed-off-by: Edward Welch <edward.welch@grafana.com>
@slim-bean slim-bean requested a review from a team as a code owner May 1, 2022 16:34
Signed-off-by: Edward Welch <edward.welch@grafana.com>
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Ooh, nice find. How'd you catch this?

pkg/ingester/tailer.go Outdated Show resolved Hide resolved
Signed-off-by: Edward Welch <edward.welch@grafana.com>
Signed-off-by: Edward Welch <edward.welch@grafana.com>
Signed-off-by: Edward Welch <edward.welch@grafana.com>
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

Nice one! LGTM!

@slim-bean slim-bean merged commit 98fda9b into main May 2, 2022
@slim-bean slim-bean deleted the tailing-single-stream branch May 2, 2022 18:34
slim-bean added a commit that referenced this pull request May 4, 2022
… not the number of streams. (#6063)"

This reverts commit 98fda9b.
slim-bean added a commit that referenced this pull request May 4, 2022
… not the number of streams. (#6063)" (#6097)

This reverts commit 98fda9b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants