-
Notifications
You must be signed in to change notification settings - Fork 838
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
feat(basic-tracer): configure logger #183
Conversation
Codecov Report
@@ Coverage Diff @@
## master #183 +/- ##
==========================================
- Coverage 93.72% 93.06% -0.66%
==========================================
Files 50 50
Lines 1609 1515 -94
Branches 100 100
==========================================
- Hits 1508 1410 -98
- Misses 101 105 +4
|
|
||
/** Constructs a new Span instance. */ | ||
constructor( | ||
parentTracer: types.Tracer, | ||
logger: Logger, |
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 put this at the end and give it a default value, e.g. the NoopLogger? Or do you think it needs to be required with no default?
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'm not a fan of hiding logs by default. Maybe it could be some kind of ConsoleLogger
by default? Then if the user prefers to use their own logger they can change it. The main issue with this approach is that console
doesn't have a log level, but that could be configured on the ConsoleLogger
with a default of ERROR
or WARN
.
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 believe that console
itself does implement our Logger
interface, and it has an implicit level of DEBUG
since it will just print all messages.
So what if we had something like this in some utility file:
export const WARN_LEVEL_CONSOLE_LOGGER = console;
I'm trying to avoid extra layers of wrapping to keep the code minimal if we can, though I'm open to it.
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'm trying to avoid extra layers of wrapping to keep the code minimal if we can, though I'm open to it.
I think the only problem with this is you can't configure the log level. I am in favor of adding ConsoleLogger
(may be same as https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-core/src/common/console-logger.ts#L25). Let me know I can it in same PR.
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'm trying to avoid extra layers of wrapping to keep the code minimal if we can
The wrapper would not do much except add the concept of a log level to console
.
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 should probably be discussed in a new issue to unblock this PR. Let's leave it required for now.
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.
Created issue to track #197
9b3839f
to
558a26b
Compare
Signed-off-by: Naseem <naseem@transit.app>
No description provided.