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

max line lengths (component + tenant overrides) #1686

Merged
merged 10 commits into from
Feb 13, 2020

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Feb 12, 2020

What

Adds flag and per tenant limits for maximum line lengths.

Also includes some refactoring for the distributor pkg to isolate logic & make it more maintainable.

Closes #1387

Distributor

distributor.max-line-size: supports sizes like 10mb, 10MB, 10Mb, 10mB, 10 MB, etc.

Tenant overrides

max_line_size: 256kb

Local testing works as expected

@owen-d owen-d changed the title Feature/line ln limits max line lengths (component + tenant overrides) Feb 12, 2020
@codecov-io
Copy link

codecov-io commented Feb 12, 2020

Codecov Report

Merging #1686 into master will decrease coverage by 0.22%.
The diff coverage is 78.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1686      +/-   ##
==========================================
- Coverage   61.86%   61.63%   -0.23%     
==========================================
  Files         116      119       +3     
  Lines        8731     8810      +79     
==========================================
+ Hits         5401     5430      +29     
- Misses       2926     2973      +47     
- Partials      404      407       +3
Impacted Files Coverage Δ
pkg/distributor/distributor.go 75% <66.66%> (+6.08%) ⬆️
pkg/util/flagext/bytesize.go 77.77% <77.77%> (ø)
pkg/distributor/validator.go 81.81% <81.81%> (ø)
pkg/promtail/targets/tailer.go 73.56% <0%> (-2.3%) ⬇️
pkg/promtail/targets/filetarget.go 68.71% <0%> (-1.85%) ⬇️
pkg/ingester/transfer.go 65% <0%> (-1.43%) ⬇️
pkg/logql/evaluator.go 91.63% <0%> (-0.65%) ⬇️
pkg/util/flagext/labelset.go 0% <0%> (ø)
... and 1 more

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.

Although I like using KB/MB I would be careful mixin unit across our full configuration. Do you support also 10000 as bytes without units ? If yes this is nice and you should add a test.

@owen-d
Copy link
Member Author

owen-d commented Feb 13, 2020

ByteSize parses bytes by default if you don't pass a duration. The actual issue with transparently moving all our configs to the new ByteSize type is that many config arguments are suffixed in an unfortunate way like distributor.ingestion-rate-limit-mb. It wouldn't really make much sense for that to suddenly default to bytes and take other units.

@owen-d owen-d merged commit d6b8587 into grafana:master Feb 13, 2020
@owen-d owen-d deleted the feature/line-ln-limits branch February 13, 2020 18:07
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.

Limits for log line length
3 participants