rollbar / rollbar.js

Error tracking and logging from Javascript to Rollbar
https://docs.rollbar.com/docs/javascript
MIT License
571 stars 213 forks source link

Continuation of issue 1084 #1088

Open npearson72 opened 1 year ago

npearson72 commented 1 year ago

@waltjones Not sure what's going on with Github (or perhaps it's an issue with this repo), but I can't seem to leave a reply on issue: https://github.com/rollbar/rollbar.js/issues/1084

It's throwing an error when I do:

Screen Shot 2023-01-05 at 3 29 35 PM

I'll post this new issue here to continue that discussion:

So given that I am not using the middleware error handler workflow, but still want to have the request object passed into error notifications to Rollbar, I wrapped my rollbar instance in a middlware function like so:

const levels = ['critical', 'debug', 'error', 'info', 'warning']

const appendRollbarInstance = (req, _res, next) => {
  for (const level of levels) {
    rollbarInstance[level] = (...args) => {
      return Rollbar.prototype[level].call(rollbarInstance, ...args, req);
    };
  }

  return next();
};

Do you see any issues with this?

The only issue I'm seeing with this approach is that you mentioned the req object can be passed in as the first argument sometimes. Currently I'm just passing in errors like so:

try {
...
} catch (err) {
  rollbarIntance.error(err)
}

But I suppose I might want to have the flexibility to pass a more specific payload like so:

try {
...
} catch (err) {
  rollbarIntance.error({ payload: { errorCode: 'some-code' } }, err)
}

Are you saying in this second example above that the req object would need to be the first argument?

waltjones commented 1 year ago

I misspoke slightly. It should usually work in any order of err, req, and custom object. Leading with the request arg can help prevent false positives on a different argument, since to detect the request, it matches on keys that can be pretty generic.

If the custom data object has for example a body key, and a request hasn't been detected yet, it will be mistakenly detected as the request. So generally, yes I would prefer to put the request before other object args and prevent this edge case.

Here's the logic for this: https://github.com/rollbar/rollbar.js/blob/master/src/utility.js#L461-L479

The simplest order that always does the right thing might be: Rollbar.prototype[level].call(rollbarInstance, req, ...args)

I'm not sure about the commenting error. It seems like a github issue, but I'll keep an eye out. Thank you for the heads up.

npearson72 commented 1 year ago

Thank you. Really appreciate your feedback.

FYI, leaving this comment by replying to the email from Github. Hopefully this works as commenting online still does not.

On Thu, Jan 5, 2023 at 4:08 PM Walt Jones @.***> wrote:

I misspoke slightly. It should usually work in any order of err, req, and custom object. Leading with the request arg can help prevent false positives on a different argument, since to detect the request, it matches on keys that can be pretty generic.

If the custom data object has for example a body key, and a request hasn't been detected yet, it will be mistakenly detected as the request. So generally, yes I would prefer to put the request before other object args and prevent this edge case.

Here's the logic for this: https://github.com/rollbar/rollbar.js/blob/master/src/utility.js#L461-L479

The simplest order that always does the right thing might be: Rollbar.prototype[level].call(rollbarInstance, req, ...args)

I'm not sure about the commenting error. It seems like a github issue, but I'll keep an eye out. Thank you for the heads up.

— Reply to this email directly, view it on GitHub https://github.com/rollbar/rollbar.js/issues/1088#issuecomment-1372908616, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKDDFTQG3X6GOJN7MWVFB3WQ5H7DANCNFSM6AAAAAATSONRCU . You are receiving this because you authored the thread.Message ID: @.***>

waltjones commented 1 year ago

Good to hear, and thank you for the follow up about the commenting issue.