-
Notifications
You must be signed in to change notification settings - Fork 70
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
cmd: log built-in-config.yaml and merged-config.yaml #202
Conversation
77c2588
to
36952e5
Compare
fa677e5
to
34a6cb8
Compare
44513bf
to
c476b08
Compare
Alternatively, we could add separate fluent bit The rule definition would be something like: With this alternative approach, we would get a single LogEntry with the entire config file (though I don't know what it would do if the input ends up in separate chunks, if the files are too large for example). I went with the simpler approach for the PR -> just logging to std out. cc'ing @davidbtucker and @quentinmit to get some thoughts around how we should self-log something like this. |
Hey. I personally don't mind the one-entry-per-line for now.
One thing though: Could we get the indentation to show up?
https://user-images.githubusercontent.com/18472685/151616324-8afc4731-b186-46ba-aaf8-a54b0bd53d1f.png
shows everything as left-aligned.
Thanks.
Dave
…On Fri, Jan 28, 2022 at 3:55 PM Ridwan Sharif ***@***.***> wrote:
Alternatively, we could add separate fluent bit head
<https://docs.fluentbit.io/manual/pipeline/inputs/head> input plugin to
read from the config files directly. And for those receivers have a multi
line processor that tries to combine everything into one LogEntry. Its a
bit tricker.
The rule definition
<https://docs.fluentbit.io/manual/administration/configuring-fluent-bit/multiline-parsing#rules-definition>
would be something like:
start_state: "# Generated by Google Cloud Ops Agent" -> A string we would
write as the first line of the config files
cont: Negative lookahead of the start_state
With this alternative approach, we would get a single LogEntry with the
entire config file (though I don't know what it would do if the input ends
up in separate chunks, if the files are too large for example).
I went with the simpler approach for the PR -> just logging to std out.
cc'ing @davidbtucker <https://github.com/davidbtucker> and @quentinmit
<https://github.com/quentinmit> to get some thoughts around how we should
self-log something like this.
—
Reply to this email directly, view it on GitHub
<#202 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJLB333DLU5XTUZNFPHYY73UYL7ELANCNFSM5D6DZ3OQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
||
// logConfigFiles logs the built-in and merged config files to STDOUT. These are then written by journald to var/log/syslog and so to | ||
// Cloud Logging once the ops-agent is running. | ||
func logConfigFiles(builtInConfigFile, mergedConfigFile string) error { |
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 wonder if it would make more sense to do this within mergeConfFiles
. Maybe see what @quentinmit thinks.
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 thought about that but since this is logged differently for Linux (stdout -> journald -> logging agent) and Windows (event log -> logging agent), I thought it probably should live outside the common code
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.
My main comment here is that we shouldn't be reading the files to log them; we should be serializing the in-memory config directly out to the log. I believe we have plans to get rid of the on-disk merged config file at some point.
I don't particularly care about formatting; we can clean that up as part of the improved/structured logging work that @davidbtucker is embarking on.
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.
Restructured some of the changes so we don't rely on reading from files and instead use the in memory structures. Additionally, I moved some code around so we don't rely on reading from these files unnecessarily.
These files are now written to just for debugging purposes.
b387dd0
to
e3af30d
Compare
The indentation is actually preserved, I see the indentation in jsonPayload but looks like Cloud Logging's UI removes leading whitespace in the quickview |
3e7fbe5
to
7be51b4
Compare
7be51b4
to
1368471
Compare
a85ad08
to
02ca99a
Compare
Had some free cycles so fixed up this PR. The PR now makes it so we no longer write the debug files to disk as we talked about. |
d0e15ac
to
859aa23
Compare
859aa23
to
5e19bd6
Compare
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.
+100, thanks for the cleanup. Still more work to do but this is already a nice improvement.
5e19bd6
to
bf319a2
Compare
This change logs the config files on both windows and linux. It does so by logging to STDOUT on linux and to the event log on windows. Additionally, it stop writing debug configs to disk. Signed-off-by: Ridwan Sharif <[email protected]>
… in-memory Signed-off-by: Ridwan Sharif <[email protected]>
bf319a2
to
70d7a2c
Compare
This change logs the config files on both windows and linux. It does so
by logging to STDOUT on linux and to the event log on windows.
This is what it looks like in cloud logging: