fluent-bit: sorted JSON and properly convert []byte to string #1310
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Jens Erat <email@jenserat.de>
While developing another fix, I stumbled upon grafana#1309 as a newly written unit test (with multiple key-value pairs in a map) was flaky. While JSON does not [strictly define an order on records in a map][RFC8259], but practical operations with a logging tool pretty much require it (although of course grep for JSON is jq, not grep...). Also, the key value output format is already sorted. Switching to sorted output in jsoniter is pretty easy. As of today, it still has a [bug] though, for which I already provided a [fix]. I propose accepting that rare case where invalid types can occur from msgpack output (can this even happen?) and re-enable the failing test case as soon as the upstream PR is merged. [RFC8259]: https://tools.ietf.org/html/rfc8259#section-4 [bug]: json-iterator/go#388 [fix]: json-iterator/go#422 Signed-off-by: Jens Erat <email@jenserat.de>
Recently, a regression was introduced that no longer ran a deep
conversion of `[]byte` to `string` unless a label map was supplied. This
commit fixes this by running the string conversion recursively, also
removing the need of applying the conversion function again during label
map stage.
This change has two minor side effects:
- Some test cases had to be moved, as string conversion happens much
earlier now.
- Invalid characters do not result in the whole label being ignored any
more, but are replaced by the unicode placeholder character now. I'd
consider this an improvement, making both debugging much easier ("why
is that value suddenly missing?") and preserving as much information
as possible.
Signed-off-by: Jens Erat <email@jenserat.de>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does / why we need it:
I'm sorry for sneaking in two changes in a single PR, but I found one of the issues while fixing the other and the code is pretty much "just next line" in the tests. Chaining PRs would be a hassle... If there are any objections regarding a single commit, I'll happily rebase it to a separate PR.
fluent-bit: fix variable spelling mistake
I guess the changes are obvious and trivial, just fixing some variable misspelled all over the code base.
fluent-bit: properly convert []byte to string
Recently, a regression was introduced that no longer ran a deep
conversion of
[]bytetostringunless a label map was supplied. Thiscommit fixes this by running the string conversion recursively, also
removing the need of applying the conversion function again during label
map stage.
This change has two minor side effects:
earlier now.
more, but are replaced by the unicode placeholder character now. I'd
consider this an improvement, making both debugging much easier ("why
is that value suddenly missing?") and preserving as much information
as possible.
fluent-bit: sort JSON map
While developing another fix, I stumbled upon #1309 as a newly written
unit test (with multiple key-value pairs in a map) was flaky.
While JSON does not strictly define an order on records in a map,
but practical operations with a logging tool pretty much require it
(although of course grep for JSON is jq, not grep...). Also, the key
value output format is already sorted.
Switching to sorted output in jsoniter is pretty easy. As of today, it
still has a bug though, for which I already provided a fix. I
propose accepting that rare case where invalid types can occur from
msgpack output (can this even happen?) and re-enable the failing test
case as soon as the upstream PR is merged.
Which issue(s) this PR fixes:
At least improves #1309. Whether this is a fix should be subject of discussion.
Special notes for your reviewer:
Checklist