Promtail: Add text encoding conversion to file targets #6395

DylanGuedes · 2022-06-14T15:37:43Z

Signed-off-by: Jaromír Wysoglad jwysogla@redhat.com

What this PR does / why we need it:
This PR adds support for text encoding conversion for file targets. To use this, just add encoding: <encoding_name> into the scrapeconfig like this:

scrape_configs:
- job_name: system_convert
  encoding: utf-16
  static_configs:
  - targets:
      - localhost
    labels:
      job: varlogs
      __path__: /var/utf16.log

The encoding name should be one of the names described by IANA here.

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

Special notes for your reviewer:
This is a revival of PR 2604. The original author couldn't work on it and suggested we continue his work.

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

Signed-off-by: Jaromír Wysoglad <jwysogla@redhat.com>

grafanabot · 2022-06-14T15:42:37Z

./tools/diff_coverage.sh ../loki-main/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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-06-14T15:47:42Z

./tools/diff_coverage.sh ../loki-main/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%

grafanabot · 2022-06-14T15:57:11Z

./tools/diff_coverage.sh ../loki-main/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.5%
+               loki	0%

cyriltovena · 2022-06-16T07:48:11Z

clients/pkg/promtail/targets/file/tailer.go

+func (t *tailer) convertToUTF8(text string) (string, error) {
+	level.Debug(t.logger).Log("msg", "Converting log encoding", "from", t.encoding, "to", "UTF8")
+
+	encoding, err := ianaindex.IANA.Encoding(t.encoding)


You should create this only once ?

done. I tested with 2 tailers and checked the source code and looks like we won't have concurrency problems reusing the decoder or the IANAEncoding struct, but do you mind checking it too?

clients/pkg/promtail/targets/file/tailer.go

…oding

grafanabot · 2022-06-16T22:39:04Z

./tools/diff_coverage.sh ../loki-main/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%

slim-bean · 2022-06-28T17:53:06Z

clients/pkg/promtail/targets/file/tailer.go

+		var text string
+		if t.decoder != nil {
+			var err error
+			text, err = t.convertToUTF8(line.Text)


what's the value for text when there is an error?

it will be "" (zero value string).

clients/pkg/promtail/targets/file/tailer.go

slim-bean · 2022-07-07T13:07:35Z

clients/pkg/promtail/targets/file/tailer.go

+			var err error
+			text, err = t.convertToUTF8(line.Text)
+			if err != nil {
+				level.Error(t.logger).Log("msg", "failed to convert encoding", "error", err)


Since we now send the error message in the log line itself, and we have a metric when this happens, I think this should be debug level, it feels mostly just likely to be noise to me at error level.

grafanabot · 2022-07-07T13:10:15Z

./tools/diff_coverage.sh ../loki-main/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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Rephrase encoding error message. Co-authored-by: Ed Welch <ed@oqqer.com>

slim-bean

LGTM

grafanabot · 2022-07-07T13:21:46Z

./tools/diff_coverage.sh ../loki-main/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.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-07-07T13:32:19Z

./tools/diff_coverage.sh ../loki-main/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%

grafanabot · 2022-07-07T13:43:46Z

./tools/diff_coverage.sh ../loki-main/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%

grafanabot · 2022-07-07T13:58:32Z

The backport to release-2.6.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-6395-to-release-2.6.x origin/release-2.6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x f17d3d768cb915b656af20a2ef7973efae335d51
# Push it to GitHub
git push --set-upstream origin backport-6395-to-release-2.6.x
git switch main
# Remove the local backport branch
git branch -D backport-6395-to-release-2.6.x

Then, create a pull request where the base branch is release-2.6.x and the compare/head branch is backport-6395-to-release-2.6.x.

Adds support for text encoding conversion for file targets. To use it, add `encoding: <encoding_name>` into the scrapeconfig. (cherry picked from commit f17d3d7)

…6395) (#6617) * Promtail: Add text encoding conversion to file targets (#6395) Adds support for text encoding conversion for file targets. To use it, add `encoding: <encoding_name>` into the scrapeconfig. (cherry picked from commit f17d3d7) * Cleanup vendor folder. * Update CHANGELOG.md Co-authored-by: Ed Welch <edward.welch@grafana.com>

Promtail: Add text encoding conversion to file targets

2234ab0

Signed-off-by: Jaromír Wysoglad <jwysogla@redhat.com>

pull-request-size bot added the size/XXL label Jun 14, 2022

github-actions bot added the area/docs label Jun 14, 2022

Add metric to report # of encoding failures.

07afd15

DylanGuedes marked this pull request as ready for review June 14, 2022 15:43

DylanGuedes requested review from KMiller-Grafana and a team as code owners June 14, 2022 15:43

cyriltovena reviewed Jun 16, 2022

View reviewed changes

clients/pkg/promtail/targets/file/tailer.go Outdated Show resolved Hide resolved

DylanGuedes added 2 commits June 16, 2022 19:05

Reuse decoder.

f44aca5

Merge branch 'main' of github.com:grafana/loki into promtail-text-enc…

d2dac86

…oding

slim-bean reviewed Jun 28, 2022

View reviewed changes

clients/pkg/promtail/targets/file/tailer.go Outdated Show resolved Hide resolved

Send error message when encoding fails.

54d3ada

slim-bean reviewed Jul 7, 2022

View reviewed changes

clients/pkg/promtail/targets/file/tailer.go Outdated Show resolved Hide resolved

slim-bean reviewed Jul 7, 2022

View reviewed changes

slim-bean added the backport release-2.6.x Tag a PR with this label to create a PR which cherry pics it into the release-2.6.x branch label Jul 7, 2022

DylanGuedes and others added 2 commits July 7, 2022 10:14

Apply suggestions from code review

64edc91

Rephrase encoding error message. Co-authored-by: Ed Welch <ed@oqqer.com>

Change encoding log level from error to debug.

015c379

slim-bean approved these changes Jul 7, 2022

View reviewed changes

Add missing CHANGELOG entry.

78bbf10

DylanGuedes merged commit f17d3d7 into grafana:main Jul 7, 2022

osg-grafana added type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories and removed area/docs labels Oct 19, 2022

This was referenced May 20, 2023

Bug: Grafana Agent mangles log files with utf-16 encoding #9488

Closed

Bug: grafana agent mangles utf-16 windows log files grafana/agent#3935

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promtail: Add text encoding conversion to file targets #6395

Promtail: Add text encoding conversion to file targets #6395

DylanGuedes commented Jun 14, 2022

grafanabot commented Jun 14, 2022

grafanabot commented Jun 14, 2022

grafanabot commented Jun 14, 2022

cyriltovena Jun 16, 2022

DylanGuedes Jun 16, 2022

grafanabot commented Jun 16, 2022

slim-bean Jun 28, 2022

DylanGuedes Jun 28, 2022

slim-bean Jul 7, 2022

grafanabot commented Jul 7, 2022

slim-bean left a comment

grafanabot commented Jul 7, 2022

grafanabot commented Jul 7, 2022

grafanabot commented Jul 7, 2022

grafanabot commented Jul 7, 2022

Promtail: Add text encoding conversion to file targets #6395

Promtail: Add text encoding conversion to file targets #6395

Conversation

DylanGuedes commented Jun 14, 2022

grafanabot commented Jun 14, 2022

grafanabot commented Jun 14, 2022

grafanabot commented Jun 14, 2022

cyriltovena Jun 16, 2022

Choose a reason for hiding this comment

DylanGuedes Jun 16, 2022

Choose a reason for hiding this comment

grafanabot commented Jun 16, 2022

slim-bean Jun 28, 2022

Choose a reason for hiding this comment

DylanGuedes Jun 28, 2022

Choose a reason for hiding this comment

slim-bean Jul 7, 2022

Choose a reason for hiding this comment

grafanabot commented Jul 7, 2022

slim-bean left a comment

Choose a reason for hiding this comment

grafanabot commented Jul 7, 2022

grafanabot commented Jul 7, 2022

grafanabot commented Jul 7, 2022

grafanabot commented Jul 7, 2022