Skip to content
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

Logger override #70

Open
edgmark opened this issue Jul 16, 2021 · 6 comments
Open

Logger override #70

edgmark opened this issue Jul 16, 2021 · 6 comments
Assignees

Comments

@edgmark
Copy link

edgmark commented Jul 16, 2021

Is it possible to override SignalFx's logger? Our app has a logger that forwards to a centralized logging platform (you know, Splunk :)). We'd like to intercept SignalFX failures and add some additional context before actually logging them. As is, we just see errors like this (for example):

error: Failed to send datapoint: 401 Unauthorized 

And without digging in, it's not obvious that it's even the SignalFx library logging these errors.

@jtmal-signalfx
Copy link
Contributor

So the obvious duck tape is below, but you should understand that if something breaks because of it, then you can't blame me.

Of course, beyond proof-of-concept, you don't want that. There's 2 ways:

  • open a support ticket (I can do it, but I need to know the contract details - is the organisation in your profile the one on behalf of which you're asking?) and we'll implement an option to expose the logger
  • send a PR that implements it, and we'll review and merge (but we'll need a CLA first)
const logger = require('signalfx/lib/logger');

const originalLoggerInfo = logger.info;
logger.info = (...args) => originalLoggerInfo.call(logger, `some context info: ${args[0]}`, ...args.slice(1));
// same pattern for other log levels: log, info, warn, warning, error, debug

const signalfx = require('signalfx');
// your code that uses signalfx

@jtmal-signalfx jtmal-signalfx self-assigned this Jul 19, 2021
@bhack-onshape
Copy link

I came to raise this exact issue and found it had already been done! We also need to be able to provide a custom logger to the signalfx module. I've taken the initiative and opened support ticket 00018228 for this since it is going to be very painful for us to have messages of mixed formats in our logs.

@jtmal-signalfx
Copy link
Contributor

@bhack-onshape Added to the backlog, I'll let you know when we have an ETA.

@bhack-onshape
Copy link

@bhack-onshape Added to the backlog, I'll let you know when we have an ETA.

Thanks @jtmal-signalfx ! I've implemented something similar to your duck tape to forward to our logger rather than console which works for now. Will be good to have core support though.

@jtmal-signalfx
Copy link
Contributor

Sorry, there was a misunderstanding and this issue was reclassified as an idea for improvement, so I opened it for you https://ideas.splunk.com/ideas/SFXMAPMID-I-152. It will need a certain number of votes to proceed.

That said the PR route is still open (but it requires https://github.com/signalfx/signalfx-nodejs/blob/main/CONTRIBUTING.md#contributor-license-agreement) and I can review PRs right after they're sent.

@andrew-prior
Copy link

The issue that I have with this is that you are throwing an unhandle-able promise rejection.

I am using the splunk-otel library, which uses this library internally. For metrics reporting, the splunk-otel library exports the metrics in a setInterval callback. When the .send method throws, the error does not get handled, and there is nothing I can do, as the consumer, to provide handling for the error.

This is all that is needed to handle the error internally: SignalFxClient.prototype.send = function (data) { var _this = this; this.rawData.push(data); return this.loadAWSUniqueId .then(function () { _this.processingData(); return _this.startAsyncSend(); }).catch(logger.error); };

With the catch block, the error gets logged, but does not register in the consumer app as an unhandled promise rejection.

I have raised an issue on the splunk-otel library as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants