Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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
Changes from 20 commits
491e6e1
18d1808
403b6a1
4c695b7
caa4507
071a13c
bb37818
728f4bf
bf30cc5
bb253b3
11d5439
fcdb38b
e01ed0f
ebdcccb
45a52eb
e80de58
3d9ef4f
fc0c481
3139761
7fd92f5
a855093
30edcbe
0edc8fb
42d664c
f2f50d6
7e7fa2f
3d11fb3
119a6a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you actually need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function's name is now a bit strange. How about renaming it to
convertLogToPlainEntry
orconvertLogToStringEntry
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, why not
convertLogBodyToEntry
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like "Body" needs the reader to have a bigger context than simply "String", for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, sounds good,
convertLogBodyToEntry
it is!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test and see what happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about what is the behavior when the timestamp is bigger than the upper boundary for
int64
, given thatlr.Timestamp
is auint64
, assuming thatlr.Timestamp
is user input. Not for this PR, as this pattern has been used before in this codebase already.