-
Notifications
You must be signed in to change notification settings - Fork 107
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
cloudapi: git sha version api and journal for workers #4484
base: main
Are you sure you want to change the base?
Conversation
default: | ||
logrus.Infof("Failed to set logging format from config, '%s' is not a valid option", config.Logging.Format) | ||
} | ||
|
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.
FYI this was copy-pasted from composer/main.go
we already use it there.
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.
Would it make sense to have some common logging setup function to actually benefit from having both composer and worker in the same repo?
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 would say no, these are completely different binaries, separated by design. Little bit of copying is sometimes better than a dependency.
} | ||
return ctx.JSON(http.StatusOK, version) | ||
} | ||
|
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 have the very same code in image-builder too, this is a copy-paste literally.
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.
Quick drive-by wondering inline
import "runtime/debug" | ||
|
||
var ( | ||
// Git SHA commit (first 4 characters) |
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 comment (4 chars) seems to not match the implementaion AFACIT
BuildCommit = "HEAD" | ||
} | ||
|
||
BuildGoVersion = bi.GoVersion |
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.
Is bi
actually defined (i.e. non-nil) if we get a !ok
before (if not we would crash here)? I wonder if it wouldn't be cleaner to define the defaults directly in var BuildCommit = "N/A"
and just return on if !ok { return }
(?)
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 major comment additionally to the already mentioned ones.
In image builder we changed the way some paths are handled so they don't need authentication, which might be neat for /version
but I'd approve as this might be easier in a followup PR
@@ -13,6 +13,12 @@ type composerConfig struct { | |||
Proxy string `toml:"proxy"` | |||
} | |||
|
|||
type loggingConfig struct { | |||
Format string `toml:"format"` // journal, text or json (defaults to jorunal or text depending on the OS) |
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.
Format string `toml:"format"` // journal, text or json (defaults to jorunal or text depending on the OS) | |
Format string `toml:"format"` // journal, text or json (defaults to journal or text depending on the OS) |
Adds a runtime information similarily as in image builder and a logrus hook that is used to put git sha to every single log message. This way we can tell which version was log created with since composer and workers sometimes are running different versions.
This data is then leveraged in a new simple /version cloud API call that returns the data similarily to IB. My plan is to call this from IB too so IB reports both its and composer version info.
Finally, I noticed that workers create pretty unreadable logs, it is a string-encoded JSON:
This patch builds on top of native journald capability which I added last year and is used in on-prem mode. Instead using standard output/err worker now detects if there is a systemd-journald running and uses its native API to send logs in structured form.
This means all logs are native now, it can be inspected with journalctl and what is more important they should get into Kibana/Splunk as regular JSON improving our searching capabilities:
I know this is sort of two features in one PR but these are tiny changes, the API endpoint is super small and I wrote a test.