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
fix(config): restore custom logger feature #299
Conversation
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.
Thanks for taking this one on!
We should probably remove debug
as a dependency as well and update the docs where we tell people to set DEBUG=...
to get debug output
lib/agent.js
Outdated
@@ -5,7 +5,7 @@ var parseUrl = require('url').parse | |||
var path = require('path') | |||
|
|||
var afterAll = require('after-all-results') | |||
var debug = require('debug')('elastic-apm') | |||
var ConsoleLogLevel = require('console-log-level') |
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's kind of a convention that variables that start with an uppercase letter like this needs to be instantiated with the new
keyword, so I'd probably change it to consoleLogLevel
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.
fixed
lib/agent.js
Outdated
this.logger = this._conf.logger || ConsoleLogLevel | ||
|
||
if (typeof this.logger === 'function') { | ||
this.logger = this.logger({ level: this._conf.logLevel }) |
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 it safe to assume that all loggers that are a function will have this API?
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 meant for this to just be a possible initialization option. If you have a logger that doesn't initialize this way, you can just initialize it externally and provide the constructed object instead.
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 thinking if a simpler API simply would be to require people to initialize their loggers before hand?
lib/parsers.js
Outdated
@@ -39,7 +38,8 @@ exports.parseMessage = function (msg) { | |||
exports.parseError = function (err, agent, cb) { | |||
stackman.callsites(err, function (_err, callsites) { | |||
if (_err) { | |||
debug('error while getting error callsites: %s', _err.message) | |||
console.log(agent) |
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.
Ups 😄
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.
fixed
lib/instrumentation/shimmer.js
Outdated
@@ -15,7 +15,7 @@ | |||
* BSD-2-Clause, http://opensource.org/licenses/BSD-2-Clause | |||
*/ | |||
|
|||
var debug = require('debug')('elastic-apm') | |||
var agent = require('../agent').singleton |
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.
Why not require('../../')
?
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.
fixed
test/parsers.js
Outdated
@@ -381,6 +381,12 @@ test('#parseError()', function (t) { | |||
_conf: { | |||
sourceLinesErrorAppFrames: 5, | |||
sourceLinesErrorLibraryFrames: 5 | |||
}, | |||
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.
Consider DRY'ing this up (is the DRY ruby'ism known in outside of the Ruby world?)
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.
fixed
test/queue.js
Outdated
@@ -7,7 +7,13 @@ var Queue = require('../lib/queue') | |||
test('maxQueueSize', function (t) { | |||
var opts = { | |||
maxQueueSize: 5, | |||
flushInterval: 1e6 | |||
flushInterval: 1e6, | |||
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.
Consider DRY'ing this up
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.
fixed
f5e2474
to
b25f917
Compare
3fba28d
to
bea6e64
Compare
bea6e64
to
9b6a671
Compare
9b6a671
to
9a8e8db
Compare
I changed it to not have the logger-as-a-function thing, so the level can just be controlled externally. The docs already partially suggested that in the custom logger section, so I just added an extra note to the |
This PR restores the custom logger feature.
Closes #289