promtail: Add `max-line-size` limit to drop on client side #8153

kavirajk · 2023-01-16T08:37:35Z

What this PR does / why we need it:
promtail: Add max-line-size limit to drop on client side

Currently the workaround is to depend only on the loki server side limits.

This PR adds support in promtail to drop "too big" log lines. It also keep track of dropped lines in promtail_dropped_entries metric with reason max_line_size_limited.

Which issue(s) this PR fixes:
Fixes #8143

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated

Currently the workaround is to depend only on the loki server side limits. This PR adds support in promtail to drop "too big" log lines. It also keep track of dropped lines in `promtail_dropped_entries` metrics with reason `max_line_size_limited`. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

grafanabot · 2023-01-16T08:45:01Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

grafanabot · 2023-01-16T09:00:01Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

clients/pkg/promtail/client/client.go

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>

grafanabot · 2023-01-16T10:18:41Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

…fana/loki into kavirajk/add-max-size-limit-promtail

grafanabot · 2023-01-16T10:39:40Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

chaudum

Otherwise ok. Approved to unblock.

chaudum · 2023-01-16T11:06:31Z

CHANGELOG.md

@@ -26,7 +26,7 @@
 * [8047](https://github.com/grafana/loki/pull/8047) **bboreham**: Dashboards: add k8s resource requests to CPU and memory panels.
 * [8061](https://github.com/grafana/loki/pull/8061) **kavirajk**: Remove circle from Loki OSS
 * [8131](https://github.com/grafana/loki/pull/8131) **jeschkies**: Compile Promtail ARM and ARM64 with journald support.
-
+* [8153](https://github.com/grafana/loki/pull/8061) **kavirajk**: promtail: Add `max-line-size` limit to drop on client side


Please move entry beneath # Promtail section.

nicoche · 2023-01-16T11:13:32Z

Hey! Thanks for the fast reaction on my issue.

I think that the distributor's setting truncates the line at that size if it's greater, but does not drop it. Should we match this in promtail? In my case, I'd rather truncate it, but I see both choices are valid.

kavirajk · 2023-01-16T13:12:30Z

I think that the distributor's setting truncates the line at that size if it's greater, but does not drop it. Should we match this in promtail? In my case, I'd rather truncate it, but I see both choices are valid.

The default behaviour is to drop it. It truncates only if distributor.max-line-size-truncate flag is set (default false)

Clarified that behaviour in the #8165.

I'm happy to add max-line-size-truncate flag to promtail as well to be consistent with distributor. But that needs introducing some additional new metrics into promtail (similar to mutated_samples_total in Loki). Will take in a different PR :)

**What this PR does / why we need it**: Add a clarrification of two flags `distributor.max-line-size` and `distributor.max-line-size-truncate`. And clarity on [discarding the logs](https://github.com/grafana/loki/blob/main/pkg/distributor/validator.go#L84-L92) if later is disabled while still former is set to non-zero values. Whereas if `distributor.max-line-size-truncate` is set, [log line is only truncated.](https://github.com/grafana/loki/blob/main/pkg/distributor/distributor.go#L517-L534) Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> **Which issue(s) this PR fixes**: Fixes #NA **Special notes for your reviewer**: Figured while working on PR #8153 **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

grafanabot · 2023-01-16T13:26:50Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

nicoche · 2023-01-16T16:22:00Z

I think that the distributor's setting truncates the line at that size if it's greater, but does not drop it. Should we match this in promtail? In my case, I'd rather truncate it, but I see both choices are valid.

The default behaviour is to drop it. It truncates only if distributor.max-line-size-truncate flag is set (default false)

Clarified that behaviour in the #8165.

I'm happy to add max-line-size-truncate flag to promtail as well to be consistent with distributor. But that needs introducing some additional new metrics into promtail (similar to mutated_samples_total in Loki). Will take in a different PR :)

Great, thank you! Is there an issue to follow this? Also, lmk if you can't find time to do that, I could submit a PR.

…8165) **What this PR does / why we need it**: Add a clarrification of two flags `distributor.max-line-size` and `distributor.max-line-size-truncate`. And clarity on [discarding the logs](https://github.com/grafana/loki/blob/main/pkg/distributor/validator.go#L84-L92) if later is disabled while still former is set to non-zero values. Whereas if `distributor.max-line-size-truncate` is set, [log line is only truncated.](https://github.com/grafana/loki/blob/main/pkg/distributor/distributor.go#L517-L534) Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> **Which issue(s) this PR fixes**: Fixes #NA **Special notes for your reviewer**: Figured while working on PR grafana#8153 **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

)

Follow up from #8153 Changes: * Change type of the flag from `int` to `flagext.ByteSize` (to be consistent with similar flag in `distributor`) * Add doc to promtail config page * Fix changelog entries Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com> **What this PR does / why we need it**: Follow up from #8153 Changes: * Change type of the flag from `int` to `flagext.ByteSize` (to be consistent with similar flag in `distributor`) * Add doc to promtail config page * Fix changelog entries **Which issue(s) this PR fixes**: Fixes #NA **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [x] `CHANGELOG.md` updated Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

pull-request-size bot added the size/L label Jan 16, 2023

kavirajk changed the title ~~promtail: Add max-line-size limit to drop on client side~~ promtail: Add max-line-size limit to drop on client side Jan 16, 2023

add test case

5850cbd

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk force-pushed the kavirajk/add-max-size-limit-promtail branch from 5410119 to 5850cbd Compare January 16, 2023 08:40

Add changelog

8197f61

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk marked this pull request as ready for review January 16, 2023 08:55

kavirajk requested a review from a team as a code owner January 16, 2023 08:55

chaudum reviewed Jan 16, 2023

View reviewed changes

clients/pkg/promtail/client/client.go Outdated Show resolved Hide resolved

clients/pkg/promtail/client/client.go Outdated Show resolved Hide resolved

Update clients/pkg/promtail/client/client.go

55b419c

Co-authored-by: Christian Haudum <christian.haudum@gmail.com>

kavirajk added 2 commits January 16, 2023 11:34

PR remarks

12062f5

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Merge branch 'kavirajk/add-max-size-limit-promtail' of github.com:gra…

e8636a0

…fana/loki into kavirajk/add-max-size-limit-promtail

kavirajk requested a review from chaudum January 16, 2023 10:44

chaudum approved these changes Jan 16, 2023

View reviewed changes

kavirajk mentioned this pull request Jan 16, 2023

doc(distributor): Clarify distributor.max-line-size flag. #8165

Merged

2 tasks

fix changelog

2ad05e4

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk merged commit 2597429 into main Jan 16, 2023

kavirajk deleted the kavirajk/add-max-size-limit-promtail branch January 16, 2023 13:48

jeschkies pushed a commit to jeschkies/loki that referenced this pull request Jan 18, 2023

promtail: Add max-line-size limit to drop on client side (grafana#8153

f4b8d4e

)

kavirajk mentioned this pull request Jan 18, 2023

promtail: Add doc for max-line-size flag. #8202

Merged

3 tasks

nicoche mentioned this pull request Jan 22, 2023

promtail: Add max-line-size-truncate #8233

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

promtail: Add `max-line-size` limit to drop on client side #8153

promtail: Add `max-line-size` limit to drop on client side #8153

kavirajk commented Jan 16, 2023 •

edited

Loading

grafanabot commented Jan 16, 2023

grafanabot commented Jan 16, 2023

grafanabot commented Jan 16, 2023

grafanabot commented Jan 16, 2023

chaudum left a comment

chaudum Jan 16, 2023

nicoche commented Jan 16, 2023

kavirajk commented Jan 16, 2023

grafanabot commented Jan 16, 2023

nicoche commented Jan 16, 2023

promtail: Add max-line-size limit to drop on client side #8153

promtail: Add max-line-size limit to drop on client side #8153

Conversation

kavirajk commented Jan 16, 2023 • edited Loading

grafanabot commented Jan 16, 2023

grafanabot commented Jan 16, 2023

grafanabot commented Jan 16, 2023

grafanabot commented Jan 16, 2023

chaudum left a comment

Choose a reason for hiding this comment

chaudum Jan 16, 2023

Choose a reason for hiding this comment

nicoche commented Jan 16, 2023

kavirajk commented Jan 16, 2023

grafanabot commented Jan 16, 2023

nicoche commented Jan 16, 2023

promtail: Add `max-line-size` limit to drop on client side #8153

promtail: Add `max-line-size` limit to drop on client side #8153

kavirajk commented Jan 16, 2023 •

edited

Loading