-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat(fe2): improved and more thorough logging to help with observability #1948
Conversation
@@ -89,7 +89,7 @@ | |||
"pg": "^8.7.3", | |||
"pg-query-stream": "^4.2.3", | |||
"pino": "^8.7.0", | |||
"pino-http": "^8.2.1", | |||
"pino-http": "^8.6.1", |
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.
9.0.0 is available if we're bumping this.
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 only updated it to fix TS type mismatches, I'd prefer not to go into the rabbit hole of doing a major upgrade in this PR, especially considering that the version has to be compatible across multiple packages (e.g. also @speckle/shared)
const endTime = Date.now() - state.start | ||
logger.info( | ||
{ | ||
responseTime: endTime, |
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.
Should we include the units in the name?: responseTimeMs
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 following the convention we have in other log messages. If we change it here, it's just going to be different from everything else, so I'd rather not
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.
- The message makes it clear that it's
ms
…ity (#1948) * better req log text * minor improvements to server logging * WIP FE2 req logging * FE2 apollo operation logging * undid apolloPlugin changes due to Gergos PR * seq message templates introduced
* gergo/apolloQueryDuration (#1949) * add apollo query duration * feat: add more details to apollo query logging * fix: pr review * feat: format log messages as clef (#1950) * fix(logging): pinoClef log levels must be a string * chore(fe2): reducing log level for some spammy req logs * minor adjustment * more robust path resolution * better req log text * feat(fe2): improved and more thorough logging to help with observability (#1948) * better req log text * minor improvements to server logging * WIP FE2 req logging * FE2 apollo operation logging * undid apolloPlugin changes due to Gergos PR * seq message templates introduced * fix: request logs (#1964) * fix: request logs * chore: remove comments * feat: add graphql subscription metrics (#1970) * optimized preview msg resultListener * fix(server): locking to avoid postgres notification listeners processing the same message multiple times (#1972) * fix(server): locking to avoid postgres notification listeners processing the same message multiple times * optimized locking * minor cleanup * msg update * log level adjustments * reduce failsafe expiry --------- Co-authored-by: Iain Sproat <68657+iainsproat@users.noreply.github.com> Co-authored-by: Kristaps Fabians Geikins <fabis94@live.com> Co-authored-by: Kristaps Fabians Geikins <fabians@speckle.systems>
Description & motivation
Changes:
To-do before merge:
Screenshots:
Validation of changes:
Checklist:
References