Skip to content
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

logfmt: don't omit last item if it does not have a value #4359

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

seveas
Copy link
Contributor

@seveas seveas commented Nov 22, 2021

A line like foo=bar baz should be parsed to {"foo"=>"bar", "baz"=>nil}, but baz was silently omitted by returning early from the parser.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

Documentation

  • [N/A] Documentation required for this feature

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@seveas
Copy link
Contributor Author

seveas commented Nov 22, 2021

With this line in foo.log

foo=bar baz

The unpatched version outputs:

$ ./build/bin/fluent-bit -R parsers.conf -f 1 -i tail -p path=foo.log -p read_from_head=true -p parser=logfmt  -o stdout -m '*'
Fluent Bit v1.9.0
* Copyright (C) 2019-2021 The Fluent Bit Authors
* Copyright (C) 2015-2018 Treasure Data
* Fluent Bit is a CNCF sub-project under the umbrella of Fluentd
* https://fluentbit.io

[2021/11/22 20:37:30] [ info] [engine] started (pid=74288)
[2021/11/22 20:37:30] [ info] [storage] version=1.1.5, initializing...
[2021/11/22 20:37:30] [ info] [storage] in-memory
[2021/11/22 20:37:30] [ info] [storage] normal synchronization mode, checksum disabled, max_chunks_up=128
[2021/11/22 20:37:30] [ info] [cmetrics] version=0.2.2
[2021/11/22 20:37:30] [ info] [sp] stream processor started
[0] tail.0: [1637609850.350374000, {"foo"=>"bar"}]
^C[2021/11/22 20:37:34] [engine] caught signal (SIGINT)
[2021/11/22 20:37:34] [ info] [input] pausing tail.0
[2021/11/22 20:37:34] [ warn] [engine] service will shutdown in max 5 seconds
[2021/11/22 20:37:35] [ info] [engine] service has stopped (0 pending tasks)

And the fixed version outputs:

$ ./build/bin/fluent-bit -R parsers.conf -f 1 -i tail -p path=foo.log -p read_from_head=true -p parser=logfmt  -o stdout -m '*'
Fluent Bit v1.9.0
* Copyright (C) 2019-2021 The Fluent Bit Authors
* Copyright (C) 2015-2018 Treasure Data
* Fluent Bit is a CNCF sub-project under the umbrella of Fluentd
* https://fluentbit.io

[2021/11/22 20:38:10] [ info] [engine] started (pid=74953)
[2021/11/22 20:38:10] [ info] [storage] version=1.1.5, initializing...
[2021/11/22 20:38:10] [ info] [storage] in-memory
[2021/11/22 20:38:10] [ info] [storage] normal synchronization mode, checksum disabled, max_chunks_up=128
[2021/11/22 20:38:10] [ info] [cmetrics] version=0.2.2
[2021/11/22 20:38:10] [ info] [sp] stream processor started
[0] tail.0: [1637609890.374907000, {"foo"=>"bar", "baz"=>nil}]
^C[2021/11/22 20:38:13] [engine] caught signal (SIGINT)
[2021/11/22 20:38:13] [ info] [input] pausing tail.0
[2021/11/22 20:38:13] [ warn] [engine] service will shutdown in max 5 seconds
[2021/11/22 20:38:14] [ info] [engine] service has stopped (0 pending tasks)

@seveas
Copy link
Contributor Author

seveas commented Nov 22, 2021

# valgrind bin/fluent-bit -R parsers.conf -f 1 -i tail -p path=foo.log -p read_from_head=true -p parser=logfmt  -o stdout -m '*'
==20522== Memcheck, a memory error detector
==20522== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==20522== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==20522== Command: bin/fluent-bit -R parsers.conf -f 1 -i tail -p path=foo.log -p read_from_head=true -p parser=logfmt -o stdout -m *
==20522==
Fluent Bit v1.9.0
* Copyright (C) 2019-2021 The Fluent Bit Authors
* Copyright (C) 2015-2018 Treasure Data
* Fluent Bit is a CNCF sub-project under the umbrella of Fluentd
* https://fluentbit.io

[0] tail.0: [1637611233.542604800, {"foo"=>"bar", "baz"=>nil}]
[2021/11/22 20:00:33] [ info] [engine] started (pid=20522)
[2021/11/22 20:00:33] [ info] [storage] version=1.1.5, initializing...
[2021/11/22 20:00:33] [ info] [storage] in-memory
[2021/11/22 20:00:33] [ info] [storage] normal synchronization mode, checksum disabled, max_chunks_up=128
[2021/11/22 20:00:33] [ info] [cmetrics] version=0.2.2
[2021/11/22 20:00:33] [ info] [sp] stream processor started
[2021/11/22 20:00:33] [ info] [input:tail:tail.0] inotify_fs_add(): inode=640070 watch_fd=1 name=foo.log
^C[2021/11/22 20:00:36] [engine] caught signal (SIGINT)
[2021/11/22 20:00:36] [ info] [input] pausing tail.0
[2021/11/22 20:00:36] [ warn] [engine] service will shutdown in max 5 seconds
[2021/11/22 20:00:37] [ info] [engine] service has stopped (0 pending tasks)
[2021/11/22 20:00:37] [ info] [input:tail:tail.0] inotify_fs_remove(): inode=640070 watch_fd=1
==20522==
==20522== HEAP SUMMARY:
==20522==     in use at exit: 0 bytes in 0 blocks
==20522==   total heap usage: 1,174 allocs, 1,174 frees, 644,671 bytes allocated
==20522==
==20522== All heap blocks were freed -- no leaks are possible
==20522==
==20522== For lists of detected and suppressed errors, rerun with: -s
==20522== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@nokute78
Copy link
Collaborator

Thank you for contribution.

A line like foo=bar baz should be parsed to {"foo"=>"bar", "baz"=>nil}

I think it should be {"foo"=>"bar", "baz"=>true}. I refered to golang implementation.
https://pkg.go.dev/github.com/kr/logfmt#pkg-overview

Note: Original PR to support logfmt is #871.
It treated true.
key%^as$=val1 %asd.5$^ a b c d e -> {"key%^as$"=>"val1", "%asd.5$^"=>true, "a"=>true, "b"=>true, "c"=>true, "d"=>true, "e"=>true}

#1737 might cause regression ?

@github-actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Mar 24, 2022
@seveas
Copy link
Contributor Author

seveas commented Sep 26, 2022

I had another look at this, and current logfmt gives all bare keys a nil value, it just misses the last one. So those are two independent bugs, and we should be able to merge this pr as-is. I'll file another pr for the nil vs true issue.

seveas added a commit to seveas/fluent-bit that referenced this pull request Sep 26, 2022
In fluent#4359, @nokute78 notes that bare keys in logfmt lines get the wrong
value after parsing: they should get a `true` value, not `nil`.
seveas added a commit to seveas/fluent-bit that referenced this pull request Sep 26, 2022
In fluent#4359, @nokute78 notes that bare keys in logfmt lines get the wrong
value after parsing: they should get a `true` value, not `nil`.

Signed-off-by: Dennis Kaarsemaker <[email protected]>
seveas added a commit to seveas/fluent-bit that referenced this pull request Sep 26, 2022
In fluent#4359, @nokute78 notes that bare keys in logfmt lines get the wrong
value after parsing: they should get a `true` value, not `nil`.

Signed-off-by: Dennis Kaarsemaker <[email protected]>
@github-actions github-actions bot removed the Stale label Sep 27, 2022
edsiper pushed a commit that referenced this pull request Oct 15, 2022
In #4359, @nokute78 notes that bare keys in logfmt lines get the wrong
value after parsing: they should get a `true` value, not `nil`.

Signed-off-by: Dennis Kaarsemaker <[email protected]>
A line like `foo=bar baz` should be parsed to `{"foo"=>"bar", "baz"=>nil}`, but `baz` was silently omitted by returning early from the parser.

Signed-off-by: Dennis Kaarsemaker <[email protected]>
@seveas seveas temporarily deployed to pr October 20, 2022 08:59 Inactive
@seveas seveas temporarily deployed to pr October 20, 2022 08:59 Inactive
@seveas seveas temporarily deployed to pr October 20, 2022 09:15 Inactive
@edsiper edsiper merged commit 8ed4c53 into fluent:master Oct 21, 2022
mgeriesa pushed a commit to mgeriesa/fluent-bit that referenced this pull request Oct 25, 2022
In fluent#4359, @nokute78 notes that bare keys in logfmt lines get the wrong
value after parsing: they should get a `true` value, not `nil`.

Signed-off-by: Dennis Kaarsemaker <[email protected]>
Signed-off-by: Manal Geries <[email protected]>
sumitd2 pushed a commit to sumitd2/fluent-bit that referenced this pull request Feb 8, 2023
In fluent#4359, @nokute78 notes that bare keys in logfmt lines get the wrong
value after parsing: they should get a `true` value, not `nil`.

Signed-off-by: Dennis Kaarsemaker <[email protected]>
Signed-off-by: root <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants