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.

Sign up for GitHub

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

Jump to bottom

Expanded Logging Customisation #1602

Closed
mod-flux opened this issue Mar 28, 2021 · 6 comments
Closed

Expanded Logging Customisation #1602

mod-flux opened this issue Mar 28, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@mod-flux
Copy link

Summary of proposed feature
Additional boolean flags in options to be used in conjunction with logger object that allows for disables the sending of log events to the _log endpoint.

Purpose of proposed feature
Serverless function invocation costs per request on Vercel. For customers that implement client and server-side exception tracking services (such as Sentry) already pay for these exceptions to be captured on said services. Having the exceptions also invoking the _log function is redundant in this scenario and would incur additional, unnecessary costs and double up on alerting.

Detail about proposed feature
Offer the ability to set _log endpoint or similar to false which disables it from being created in Vercel and disables the client and server parts of next-auth from attempting to POST data to the endpoint when an exception is caught. Customers are then encouraged to use the pre-existing logger object to capture and alert with their own logging platform.

Potential problems
None that I can think of.

Describe any alternatives you've considered
Attempting to link Sentry to DataDog to capture _log events but this seems backward.

Additional context
I can obviously try and help with this implementation.
@mod-flux mod-flux added the enhancement New feature or request label Mar 28, 2021
@balazsorban44
Copy link
Member

balazsorban44 commented Mar 28, 2021 •
edited
Loading

Thank you for opening. So adding a boolean to log invocations to be able differentiate between client and server logs are easy, but to set an option to don't send logs from the client at all is a bit harder. There might be an option to set it in the Provider as a property in the options prop, but that would probably require some potentially breaking refactoring. I need to think about it.

Merging #1473 would make this easier to implement, as the frontend code would be more tied to React.

@mod-flux
Copy link
Author

Thanks for responding @balazsorban44, would this move to Provider also affect custom OAuth clients as well as pre-configured ones? Had a look at the code changes but couldn't determine.

Ideally not sending it at all would be great as redundant serverless invocations wouldn't be necessary. However, even providing some context as to where an error has originated from within the logger callbacks would be a good step.

@balazsorban44
Copy link
Member

balazsorban44 commented Mar 28, 2021 •
edited
Loading

It would be nice to have @iaincollins's opinion on #1473 for this to be resolved properly (and potentially other issues while also optimizing performance), but adding a simple frontend: true boolean to client-side logs shouldn't be hard. This will still make an invocation though, but frontend logs won't be reported twice. Any interest in implementing it? Either in the server/index.js _log case, or in the proxyLogger function in lib/logger.

I got curious though, how does sentry's client logger catch the frontend errors created by next-auth?

@stale
Copy link

stale bot commented May 27, 2021

Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label May 27, 2021
@balazsorban44
Copy link
Member

balazsorban44 commented May 28, 2021 •
edited
Loading

There hasn't been much interest in this from the community. Adding a frontend flag is still on the table for me, but since these logs should only be sent when something is not working correctly, I don't think a high number of function invocations would occur.

If you are interested in opening a PR addressing the frontend flag, please do.

@stale stale bot removed the stale Did not receive any activity for 60 days label May 28, 2021
@balazsorban44 balazsorban44 mentioned this issue Jul 10, 2021
Merged
@balazsorban44
Copy link
Member

balazsorban44 commented Jul 10, 2021 •
edited
Loading

This will be closed by #2344
It adds a client: true flag, when the log event comes from the frontend.

As mentioned, the log events will still happen, but you can filter them out in the logger instance by checking for the client flag.

This is an easy fix, and as I said, if you see a high number of invocations of that _log endpoint, you have bigger problems than unnecessary serverless calls. (Since we only log errors from the frontend that means that your client-side auth handling does not work)

balazsorban44 added a commit that referenced this issue Jul 11, 2021
@balazsorban44
feat(logger): simplify logger API (#2344)
acc9393 
Similar to #2342, this aims to unify the user-facing API and provide an easier way to extend in the future.

In addition, this PR also solves the problem when the `logger.error` method sometimes did not print results, because `Error` instances are not serializable and will be printed as empty objects `"{}"`.

After this PR, we make any `Error` instances serializable as described here: https://iaincollins.medium.com/error-handling-in-javascript-a6172ccdf9af

Closes #1602
Achieved by adding a `client: true` flag when logs are coming from the frontend.

BREAKING CHANGE:

The main change is that instead of an unknown number of parameters, the log events have at most two, where the second parameter is usually an object. In the case of the `error` event, it can also be an `Error` instance (that is serializable by `JSON.stringify`). If it is an object, an `Error` instance will be available on `metadata.error`, and `message` will default to `metadata.error.message`. This is done so that an error event always provides some kind of a stack to see where the error happened

```diff
// [...nextauth.js]
import log from "some-logger-service"
...
logger: {
- error(code, ...message) {},
+ error(code, metadata) {},
- warn(code, ...message) {},
+ warn(code) {}
- debug(code, ...message) {}
+ debug(code, metadata) {}
}
```
mnphpexpert added a commit to mnphpexpert/next-auth that referenced this issue Sep 2, 2024
@mnphpexpert
feat(logger): simplify logger API (nextauthjs#2344)
94a6c70 
Similar to nextauthjs#2342, this aims to unify the user-facing API and provide an easier way to extend in the future.

In addition, this PR also solves the problem when the `logger.error` method sometimes did not print results, because `Error` instances are not serializable and will be printed as empty objects `"{}"`.

After this PR, we make any `Error` instances serializable as described here: https://iaincollins.medium.com/error-handling-in-javascript-a6172ccdf9af

Closes nextauthjs#1602
Achieved by adding a `client: true` flag when logs are coming from the frontend.

BREAKING CHANGE:

The main change is that instead of an unknown number of parameters, the log events have at most two, where the second parameter is usually an object. In the case of the `error` event, it can also be an `Error` instance (that is serializable by `JSON.stringify`). If it is an object, an `Error` instance will be available on `metadata.error`, and `message` will default to `metadata.error.message`. This is done so that an error event always provides some kind of a stack to see where the error happened

```diff
// [...nextauth.js]
import log from "some-logger-service"
...
logger: {
- error(code, ...message) {},
+ error(code, metadata) {},
- warn(code, ...message) {},
+ warn(code) {}
- debug(code, ...message) {}
+ debug(code, metadata) {}
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@balazsorban44 @mod-flux