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

Promtail: (and also fluent-bit) change the max batch size to 1MB #2710

Merged
merged 2 commits into from
Oct 1, 2020

Conversation

slim-bean
Copy link
Collaborator

This is a follow up to the change to increase batch size made in #2618 but to also apply it in the handful of other places we have the defaults specified. I tried to centralize this a little but more work needs to be done stop defining defaults in helm and fluent-bit...

…and fluent-bit, attempt to centralize this config a little where possible.
@@ -11,6 +11,16 @@ import (
lokiflag "github.com/grafana/loki/pkg/util/flagext"
)

// NOTE the helm chart for promtail and fluent-bit also have defaults for these values, please update to match if you make changes here.
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure we could find a way at some point to have global default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

helm makes this difficult, we specify the values in the config so they can be overridden but i think leaving them empty would be an error, I don't have the time to dig deeper into this right now so I added this comment as "better than nothing"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep makes total sense.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #2710 into master will decrease coverage by 0.05%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2710      +/-   ##
==========================================
- Coverage   61.37%   61.32%   -0.06%     
==========================================
  Files         173      173              
  Lines       13444    13444              
==========================================
- Hits         8251     8244       -7     
- Misses       4440     4444       +4     
- Partials      753      756       +3     
Impacted Files Coverage Δ
cmd/docker-driver/config.go 9.34% <0.00%> (ø)
pkg/promtail/client/config.go 55.55% <50.00%> (ø)
pkg/promtail/targets/file/tailer.go 68.88% <0.00%> (-4.45%) ⬇️
pkg/promtail/targets/file/filetarget.go 64.08% <0.00%> (-2.12%) ⬇️

@slim-bean slim-bean merged commit d3bf21e into master Oct 1, 2020
@slim-bean slim-bean deleted the promtail-batch-size branch October 1, 2020 18:46
slim-bean added a commit that referenced this pull request Oct 1, 2020
* change the max batch size to 1MB for all the defaults including helm and fluent-bit, attempt to centralize this config a little where possible.

* fix test

(cherry picked from commit d3bf21e)
owen-d pushed a commit that referenced this pull request Jul 8, 2021
…-date defaults (#3559)

* Doc: Remove removed --ingester.recover-from-wal option

Removed in:
8a9b94a ("removes recover flag (#3326)")

* Doc: Fix out-of-date defaults

0b1dbe2 ("Promtail: Add a stream lagging metric (#2618)")
d3bf21e ("Promtail: (and also fluent-bit) change the max batch size to 1MB  (#2710)")
mraboosk pushed a commit to mraboosk/loki that referenced this pull request Oct 7, 2024
…fana#2710)

* change the max batch size to 1MB for all the defaults including helm and fluent-bit, attempt to centralize this config a little where possible.

* fix test
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