-
Notifications
You must be signed in to change notification settings - Fork 19
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
Refactor metrics logger #176
Conversation
819f640
to
e21fa77
Compare
e21fa77
to
aedfc78
Compare
Looks like https://github.com/go-kit/kit/tree/master/log doesn't support async logging, which we will surely need. @ordovicia @everpeace So for the time being, what do you think this refactoring? Do you think merging file metrics logger and stdout metrics loggers is not intuitive? |
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 can now remove stdout_writer.go
.
For now, merging the file writer and stdout writer sounds good to me.
For a future possibility, we can modify the writer to take io.Writer
interface instead of a concrete type os.File
.
pkg/metrics/file_writer.go
Outdated
if err != nil { | ||
return nil, err | ||
var file *os.File | ||
if path == "/dev/stdout" || path == "" { |
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.
IMHO it is not good to implicitly set the destination to stdout when path
is empty.
I prefer to explicitly specify /dev/stdout
.
pkg/config/config.go
Outdated
type MetricsStdoutConfig struct { | ||
type MetricsLoggerConfig struct { | ||
// Path is a file path in which the metrics is written. | ||
Path string |
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.
It is not necessarily a good idea, but we could accept stdout
as well as /dev/stdout
(and stderr
) for convenience as the output destination.
If we take this way, the name Path
should be changed, since stdout
is not a path.
@ordovicia I updated the code. Could you check it again when you have time?
I agree with this idea, and I tried to implement it, however, I wasn't able to find a good and simple solution to avoid the file descriptor leak. I think we can make it in another PR if necessary. |
db09441
to
08e5a88
Compare
example/config.yaml
Outdated
@@ -14,20 +14,18 @@ startClock: 2019-01-01T00:00:00+09:00 | |||
# Optional (default: same as tick) | |||
metricsTick: 60 | |||
|
|||
# Metrics of simulated kubernetes cluster is written to a file at this path. | |||
# Metrics of simulated kubernetes cluster is written | |||
# to a file at this path or standard out if not specified. |
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.
standard out if not specfiied
Could you please update this comment?
After this line is fixed, I will merge this PR. Thank you!
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.
Thank you.
The last some commits are by me. I believe they didn't introduce regression.
I refactored the metrics logger a little bit. It can be the start point of issue #145, or this change may be enough.