feat(payments-next): Improve payments-next logging #18869
Conversation
StaberindeZA
left a comment
StaberindeZA
left a comment
There was a problem hiding this comment.
There's a couple of console statements in payments-next and with sanitizeExceptions as well. Could you have a look at those as well please?
david1alvarez
force-pushed
the
FXA-11639
branch
4 times, most recently
from
9a58137 to
82213e3
Compare
StaberindeZA
left a comment
StaberindeZA
left a comment
There was a problem hiding this comment.
In the image below, I've noticed a few issues, which I've listed below.
- ERROR logs aren't including the additional data. The LOG and ERROR statements are the same in the code,
- The error property is meant to be an Error object, but only shows an empty object instead
- Is this the expected format for parsing into BigQuery?
| AccountDatabaseNestFactory, | ||
| CartManager, | ||
| LegacyStatsDProvider, | ||
| { provide: Logger, useValue: winstonLogger }, |
There was a problem hiding this comment.
Does fxa-admin-server use winstonLogger in the rest of the app? If not, then the same logger should be reused here, as is in the rest of the app.
There was a problem hiding this comment.
Looks like fxa-admin-server mostly uses the MozLoggerService, though predominantly this will be used in the CartManager, which is imported here for use in the admin panel. I'll make the change for consistency with the admin panel though
david1alvarez
force-pushed
the
FXA-11639
branch
7 times, most recently
from
bde3b1d to
2ad6f05
Compare
| this.log.error('profileClient.deleteCache.failed', { | ||
| uid, | ||
| error: err.toString(), | ||
| }); |
There was a problem hiding this comment.
I thought for log.error, the first argument needs to be an Error object?
There was a problem hiding this comment.
It does, yes. I'll go through the PR this morning to see if I can catch anything else. It's changed a bit over the time its been worked on haha
There was a problem hiding this comment.
Ah yes fair enough. Sorry for jumping in a bit early.
There was a problem hiding this comment.
This still looks open?
StaberindeZA
left a comment
StaberindeZA
left a comment
There was a problem hiding this comment.
r+wc. Just 2 comments, but otherwise lets get this merged. Thank you!
| info, | ||
| }); | ||
| this.name = 'CartError'; | ||
| Object.setPrototypeOf(this, CartError.prototype); |
There was a problem hiding this comment.
Just curious and for logging purposes, why is this necessary?
There was a problem hiding this comment.
Object.setPrototype is used for instanceof checks, which comes into play for several of our tests. instanceof check are inconsistent otherwise
| this.log.error('profileClient.deleteCache.failed', { | ||
| uid, | ||
| error: err.toString(), | ||
| }); |
There was a problem hiding this comment.
This still looks open?
Because: * Current logs to bigquery from payments next are malformed and not queriable This commit: * Addresses formatting issues of the logs Closes #FXA-11639
| this.log.error('profile.deleteCache.failed', uid, err); | ||
| throw err; | ||
| } catch (error) { | ||
| this.log.error(error); |
There was a problem hiding this comment.
nit. ideally this would be something like the code below, so that the additional context of the uid isn't lost. This can be fixed up at a later stage though.
const deleteCacheError = new ProfileClientDeleteCacheError(
'Error occurred during delete cache',
uid,
error
)
this.log.error(deleteCacheError);
throw deleteCacheError;
Because:
This commit:
Closes #FXA-11639
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.