Loki Exporter - Adding a feature for loki exporter to encode JSON for log entry #5846

gillg · 2021-10-20T23:08:10Z

Originaly from #3874

Description:

The PR creates a feature for the Loki Exporter that allows you to use a JSON representation of the LogRecord as described by https://developers.google.com/protocol-buffers/docs/proto3#json.

It creates a Loki LogEntry using JSON instead of the current body string.

The format option allows you to choose between json or the body string, the default is the body string.

This feature allows us to handle structured logging the Loki exporter and use structured logging in our logging pipeline.

Link to tracking Issue: This may fix the issue: #2529

Testing: TestConvert - Ensuring the json encoding is providing expected responses.

TestJsonLoadConfig - Ensuring the format config option is set correctly.

TestExporter_convertLogtoJsonEntry - Ensuring the Log Entry is converted to a JSON Entry.

Documentation: Comments added to the functions and README options updated.

…metry-collector-contrib into improve-loki

…y-collector-contrib into improve-loki

gillg · 2021-10-20T23:11:11Z

@crearys I recreated your PR, added some little changes to "finish" a v1, so even if it's probably not perfect it covers probably almost 100% of use cases.
I would prefer merge it than wait for solving all conceptual questions in the other PR.

In the meantime I try to find a maintener for the exporter, and be able to merge !

gillg · 2021-10-20T23:12:43Z

PR in draft, I'm searching for a Loki maintainer on "loki-dev" slack

crearys · 2021-10-21T09:46:58Z

@gillg Apologies for the slow reply, i've been really busy recently. I totally agree as we are still using a forked version with this feature. It would be good to get this merged once we have a new maintainer.

gillg · 2021-10-25T09:07:11Z

@jpkrohling proposed to adopt the orphan exporter Loki 👏

jpkrohling

Just a couple of minor items, but looks good to me overall!

exporter/lokiexporter/config_test.go

exporter/lokiexporter/README.md

jpkrohling · 2021-10-25T09:37:48Z

exporter/lokiexporter/example/otel-collector-config.yml


 extensions:
  health_check:
  pprof:
  zpages:
+  memory_ballast:


Do you actually need this?

No, but was to be consistent with the previous exemple. We can remove it if you think it's better.
Does memory ballast shouldn't always be used when we receive pushed data like logs ?

We recommend that people always use things like the batch processor and memory ballast, but I find it confusing to add something not relevant to what is being demonstrated.

exporter/lokiexporter/exporter.go

jpkrohling · 2021-10-25T09:43:53Z

exporter/lokiexporter/exporter.go

@@ -195,9 +210,20 @@ func (l *lokiExporter) convertAttributesToLabels(attributes pdata.AttributeMap,
 	return ls
 }

-func convertLogToLokiEntry(lr pdata.LogRecord) *logproto.Entry {
+func convertLogToLokiEntry(lr pdata.LogRecord, res pdata.Resource) (*logproto.Entry, error) {


This function's name is now a bit strange. How about renaming it to convertLogToPlainEntry or convertLogToStringEntry?

Agree, why not convertLogBodyToEntry ?

I feel like "Body" needs the reader to have a bigger context than simply "String", for instance.

Hum, I don't understand you you mean, but by the way.... I'm thinking the body can be a complex structure in OTEL, I already had a case here #4955
I was trying to use the syslog receiver, but all structured data are put in the body instead of attributes, and attributes are empty.

As opposite, fluentforward receiver set an empty body (except if it finds an arbitrary key "message" or "log") and put all structured log data in attributes.

Alright, sounds good, convertLogBodyToEntry it is!

OK !
But does lr.Body().StringVal() will handle a structured body ? (case where you use syslog receiver)
If not we need to serialize it anyway :/
For the JSON encoding case it's implicit, but for this "raw string" output it's not obvious. Does a logfmt serializer exists ?

Can you add a test and see what happens?

gillg · 2021-10-25T15:49:13Z

Last question @jpkrohling.
Maybe we can validate this PR to move forward (once review fixed), but I have some conceptual points from my previous review in the old PR.

With the format "body", the loki log entry will be something like : {"name":"name","body":"Example log","traceid":"01020304000000000000000000000000","spanid":"0506070800000000","severity":"error","attributes":{"attr1":"1","attr2":"2"},"resources":{"attr1":"1","attr2":"2"}} so we have nested structures for attributes and resources.
I didn't tested it, it seems supported by Loki (it flattens everything with json pipe) and Grafana (the rendering seems supported ?).
So in theory something like {...} | json | attributes_attr1="1" is possible, but is it a good practice to push a JSON with nested structures or should we flaten it before sending it ? The final query will be the same, but I was wondering for perfs reasons.

Note : Indexed labels are not related to the log entry, we create the mapping manualy from pure OTEL attributes and resources.

cyriltovena · 2021-10-26T10:21:56Z

Last question @jpkrohling. Maybe we can validate this PR to move forward (once review fixed), but I have some conceptual points from my previous review in the old PR.

With the format "body", the loki log entry will be something like : {"name":"name","body":"Example log","traceid":"01020304000000000000000000000000","spanid":"0506070800000000","severity":"error","attributes":{"attr1":"1","attr2":"2"},"resources":{"attr1":"1","attr2":"2"}} so we have nested structures for attributes and resources. I didn't tested it, it seems supported by Loki (it flattens everything with json pipe) and Grafana (the rendering seems supported ?). So in theory something like {...} | json | attributes_attr1="1" is possible, but is it a good practice to push a JSON with nested structures or should we flaten it before sending it ? The final query will be the same, but I was wondering for perfs reasons.

Note : Indexed labels are not related to the log entry, we create the mapping manualy from pure OTEL attributes and resources.

No problem with JSON, I prefer when logs are human readable but that seems to be a valid option to push json if we want to keep metadata inside each line.

May be we should add another format like logfmt too in the future which is more human readable.

gillg · 2021-10-26T11:08:28Z

No problem with JSON, I prefer when logs are human readable but that seems to be a valid option to push json if we want to keep metadata inside each line.

May be we should add another format like logfmt too in the future which is more human readable.

Thanks @cyriltovena !
I aggree JSON is not ideal, but it's not so bad at the end until we have something structured. I use format_line() on dashboards and the result is clear.
Maybe a YAML format could be a compromise between JSON and logfmt... As an exemple, a stacktrace in logfmt seems not more easy to read without carriage returns or indentation

jpkrohling · 2021-10-27T09:21:38Z

exporter/lokiexporter/exporter.go

@@ -150,6 +178,8 @@ func (l *lokiExporter) logDataToLoki(ld pdata.Logs) (pr *logproto.PushRequest, n
 		}
 	}

+	l.logger.Error("some logs has been dropped", zap.Error(errs))


This seems to be lost here. Shouldn't you wrap it in a conditional?

jpkrohling · 2021-10-27T09:27:25Z

exporter/lokiexporter/exporter.go

 	}
+	return &logproto.Entry{
+		Timestamp: time.Unix(0, int64(lr.Timestamp())),


I'm curious about what is the behavior when the timestamp is bigger than the upper boundary for int64, given that lr.Timestamp is a uint64, assuming that lr.Timestamp is user input. Not for this PR, as this pattern has been used before in this codebase already.

github-actions · 2021-11-04T05:15:26Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-11-04T08:32:05Z

@gillg, do you need any help progressing with this PR?

gillg · 2021-11-05T21:10:49Z

@gillg, do you need any help progressing with this PR?

Hello, yes I have a lack of time and... I'm sure in fact that the actual body serialization is not valid. It could work as a v1 but knowing it doesn't cover use cases where your body is a complex object and not a simple string. 😕

What do you want ? We assume I just fix remaining details or we target the "perfect" version ?

P.S: I can make functional changes, but I'm probably not confortable enought with Go to create many tests for different complex use tests with basic attributes, basic resources, string body, complex body, etc. I don't have any idea how to serialize in a generic way a pdata because it can contains string, AttributeMap, ints, etc.

jpkrohling · 2021-11-08T08:30:59Z

I aggree JSON is not ideal, but it's not so bad at the end until we have something structured. I use format_line() on dashboards and the result is clear.

I think this is the key: you are adding a feature that solves a problem for you, and it seems reasonable to me. Let's get this first version done, and iterate from there.

About further testing: create an issue to track it and assign it to me. I can take a look and contribute some tests, tagging you to review my work. How does that sound?

github-actions · 2021-11-16T05:15:33Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-11-23T05:15:37Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

gillg · 2021-12-04T14:13:48Z

Hello !
Sorry I was completely unavailable these last weeks...
I just made a new commit to fix some remaining things, @jpkrohling can you reopen the PR please ? 😅

gillg · 2021-12-04T15:00:32Z

By the way have you some informations about this otep ? open-telemetry/oteps#188
It seems abandoned, but I don't really understand why.
Force some consistency seems not a bad idea... As exemple the syslog receiver is putting all metadata in a body map, but there is no processor able to work with body attributes, and probably 80% of exporters will don't handle them.

…y-collector-contrib into lokiencoding-pr

jpkrohling

This is looking good! Just a question, but I think it's ready to be merged.

exporter/lokiexporter/exporter.go

jpkrohling · 2021-12-06T16:33:19Z

@cyriltovena, would you like to take a final look?

…a trace of the reason

jpkrohling

LGTM. I'll wait until tomorrow afternoon (UTC) to see if @cyriltovena wants to give a final review, otherwise I'll merge this.

crearys and others added 19 commits May 21, 2021 14:59

initial commit

491e6e1

revert un-needed changes

18d1808

test json encode as entry

403b6a1

add encoding files

4c695b7

merge loki encoding with the resource atttribute branch

caa4507

add format option to loki exporter config

071a13c

add test for loki/json test data

bb37818

improve variable names and comments

728f4bf

pull from upstream main and update loki exporter files to fix tests

bf30cc5

Create feature for loki exporter to encode json

bb253b3

mapping convert functions

11d5439

Fix linting errors

fcdb38b

Merge remote-tracking branch 'upstream/main' into lokiencoding-pr

e01ed0f

update pdata package

ebdcccb

pull upstream

45a52eb

update to use otel 0.36

e80de58

Merge branch 'lokiencoding-pr' of https://github.com/crearys/opentele…

3d9ef4f

…metry-collector-contrib into improve-loki

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

fc0c481

…y-collector-contrib into improve-loki

Add resources in encoded json structure

3139761

Add missing licence

7fd92f5

jpkrohling self-requested a review October 21, 2021 12:11

gillg marked this pull request as ready for review October 25, 2021 09:05

gillg requested a review from a team October 25, 2021 09:05

github-actions bot assigned owais Oct 25, 2021

jpkrohling reviewed Oct 25, 2021

View reviewed changes

Group similar errors

30edcbe

jpkrohling reviewed Oct 27, 2021

View reviewed changes

github-actions bot added the Stale label Nov 4, 2021

github-actions bot removed the Stale label Nov 5, 2021

github-actions bot added the Stale label Nov 16, 2021

github-actions bot closed this Nov 23, 2021

Fix serialization

0edc8fb

Add a test for unsuported body type

42d664c

gillg added 2 commits December 4, 2021 16:18

Fix tests

f2f50d6

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

7e7fa2f

…y-collector-contrib into lokiencoding-pr

mx-psi reopened this Dec 6, 2021

Fix lint

3d11fb3

jpkrohling reviewed Dec 6, 2021

View reviewed changes

exporter/lokiexporter/exporter.go Outdated Show resolved Hide resolved

Change error level to debug for dropped logs to avoid flood, keeping …

119a6a6

…a trace of the reason

jpkrohling approved these changes Dec 6, 2021

View reviewed changes

github-actions bot removed the Stale label Dec 7, 2021

jpkrohling merged commit 2c54472 into open-telemetry:main Dec 7, 2021

gregoryfranklin mentioned this pull request Dec 8, 2021

lokiexporter: support serialization of all body types into JSON #6639

Merged

kevinschoonover mentioned this pull request Dec 11, 2021

journald: use body["MESSAGE"] for entry body open-telemetry/opentelemetry-log-collection#327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki Exporter - Adding a feature for loki exporter to encode JSON for log entry #5846

Loki Exporter - Adding a feature for loki exporter to encode JSON for log entry #5846

gillg commented Oct 20, 2021

gillg commented Oct 20, 2021

gillg commented Oct 20, 2021

crearys commented Oct 21, 2021

gillg commented Oct 25, 2021

jpkrohling left a comment

jpkrohling Oct 25, 2021

gillg Oct 25, 2021

jpkrohling Oct 26, 2021

jpkrohling Oct 25, 2021

gillg Oct 25, 2021

jpkrohling Oct 26, 2021

gillg Oct 26, 2021 •

edited

Loading

jpkrohling Oct 26, 2021

gillg Oct 26, 2021

jpkrohling Oct 27, 2021

gillg commented Oct 25, 2021

cyriltovena commented Oct 26, 2021

gillg commented Oct 26, 2021

jpkrohling Oct 27, 2021

jpkrohling Oct 27, 2021

github-actions bot commented Nov 4, 2021

jpkrohling commented Nov 4, 2021

gillg commented Nov 5, 2021

jpkrohling commented Nov 8, 2021

github-actions bot commented Nov 16, 2021

github-actions bot commented Nov 23, 2021

gillg commented Dec 4, 2021

gillg commented Dec 4, 2021

jpkrohling left a comment

jpkrohling commented Dec 6, 2021

jpkrohling left a comment

Loki Exporter - Adding a feature for loki exporter to encode JSON for log entry #5846

Loki Exporter - Adding a feature for loki exporter to encode JSON for log entry #5846

Conversation

gillg commented Oct 20, 2021

gillg commented Oct 20, 2021

gillg commented Oct 20, 2021

crearys commented Oct 21, 2021

gillg commented Oct 25, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gillg Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gillg commented Oct 25, 2021

cyriltovena commented Oct 26, 2021

gillg commented Oct 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 4, 2021

jpkrohling commented Nov 4, 2021

gillg commented Nov 5, 2021

jpkrohling commented Nov 8, 2021

github-actions bot commented Nov 16, 2021

github-actions bot commented Nov 23, 2021

gillg commented Dec 4, 2021

gillg commented Dec 4, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Dec 6, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

gillg Oct 26, 2021 •

edited

Loading