[logp] Default logging config logs to file #208
Conversation
belimawr
added
the
Team:Elastic-Agent-Data-Plane
belimawr
requested review from
AndersonQ and
rdner
and removed request for
a team
belimawr
force-pushed
the
log-to-file-by-default
branch
2 times, most recently
from
c3cb526 to
cc05474
Compare
pierrehilbert
requested review from
faec and
leehinman
and removed request for
AndersonQ and
rdner
belimawr
force-pushed
the
log-to-file-by-default
branch
from
cc05474 to
f5d2ca3
Compare
The default log configuration must log to files. A test is added to ensure the default logging configuration logs to files as well as some other default fields are correctly set.
belimawr
force-pushed
the
log-to-file-by-default
branch
from
f5d2ca3 to
9a04ff4
Compare
💚 Build Succeeded
History
|
| _, fileName, lineNum, _ := runtime.Caller(0) | ||
| lineNum-- // We want the line number from the log | ||
| fileName = filepath.Base(fileName) |
There was a problem hiding this comment.
Why do we need this here? This is really a test of what the logger.Info calls does isn't it?
The point of this test is to prove that the default configuration logs to files, not to also test exact format of the logs themselves? Is that correct?
I think I am mostly concerned that this test succeeding currently depends on the exact placement of these lines in the file. It seems unnecessary to maintain this to verify what you need to.
There was a problem hiding this comment.
The point of this test is to prove that the default configuration logs to files, not to also test exact format of the logs themselves? Is that correct?
Yes, but as I wrote the test I thought it would be nice to ensure some other fields we expect are there as well. The file and line number are essential exactly which bit of the code is producing the log entry. They're the first thing I look at when I'm dealing with a log entry I'm not used to see.
I don't mind removing this assertion, however I believe we should have a test that ensures our expected default behaviour, which includes some essential fields.
I think I am mostly concerned that this test succeeding currently depends on the exact placement of these lines in the file. It seems unnecessary to maintain this to verify what you need to.
I get that, hence I'm reading them from the runtime instead of just hard-coding them. As long as L52-54 are always together, no matter where in the file the tests will succeed.
I prefer adding a comment clarifying what those lines are doing and stating they must stay together rather than removing this assertion.
However, if you insist, I'm not against removing them for now and later writing a more comprehensive test with our expectations for a default log entry.
There was a problem hiding this comment.
I don't hate it, I just wanted to sanity check how necessary this was. The root of my concern is that this could be fragile, and also if this functionality is already tested as a core part of the upstream zap logger it uses underneath.
It's cheap enough to have here, it is not worth going on a separate adventure to perfectly test every aspect of the logger right now.
What does this PR do?
The default log configuration must log to files. A test is added to ensure the default logging configuration logs to files as well as some other default fields are correctly set.
Why is it important?
It fixes the default behaviour that was broken by #205
Checklist
Author's Checklist
## Related issues