Add structured logging and Prometheus events #1767
Conversation
| @@ -1,38 +1,46 @@ | |||
| import * as dotenv from 'dotenv' | |||
|
|
|||
| // We need to load the config before some of the imports for e.g. the logger | |||
| if (process.env.CONFIG) { | |||
There was a problem hiding this comment.
this doesn't sound right... imports by ES6 definition must be the first thing on a file, you can't add imports after code
| import { initializeDB, initializeKit } from './db' | ||
| import { createValidatedHandler } from './request' | ||
| import { logger } from './logger' |
There was a problem hiding this comment.
maybe rename to "rootLogger"
| }) | ||
|
|
||
| // Somehow it wouldn't let me export logger without export Logger | ||
| export { Logger } |
There was a problem hiding this comment.
try setting the type for const logger explicitely
const logger: Logger
| import express from 'express' | ||
| import { isLeft } from 'fp-ts/lib/Either' | ||
| import * as t from 'io-ts' | ||
| import { logger } from './logger' | ||
|
|
||
| export enum ErrorMessages { | ||
| UNKNOWN_ERROR = 'Something went wrong', | ||
| } | ||
|
|
||
| export function catchAsyncErrorHandler<T>( |
There was a problem hiding this comment.
i think asyncHandler() is good enough name :P
| @@ -64,3 +68,22 @@ function serializeErrors(errors: t.Errors) { | |||
| export function respondWithError(res: express.Response, statusCode: number, error: string) { | |||
| res.status(statusCode).json({ success: false, error }) | |||
| } | |||
|
|
|||
| export type Response = Omit<express.Response, 'locals'> & { | |||
| locals: { logger: Logger } & any | |||
There was a problem hiding this comment.
I wasn't entirely sure if the compiler would be capable to intersect both locals, but let me try
There was a problem hiding this comment.
Turns out
export type Response = Omit<express.Response, 'locals'> & {
locals: { logger: Logger } & Omit<any, 'logger'>
}
was the way to go
| respondWithError(res, 500, ErrorMessages.UNKNOWN_ERROR) | ||
| } | ||
| return (req: express.Request, res: Response) => { | ||
| handler(req, res) |
There was a problem hiding this comment.
word of caution.... here you don't catch an edge case, suppose the handler() do some things and then return a promise... for example:
function evilHandler() {
if (true) {
throw new Error()
}
return callSomethingThatRetunsAPromise()
}In this case... there's a throw before, you are not actually catching the error.
There was a problem hiding this comment.
Great catch, definitely wasn't aware
| @@ -31,6 +31,8 @@ | |||
| "@celo/utils": "^0.1.0", | |||
| "bignumber.js": "^7.2.0", | |||
| "body-parser": "1.19.0", | |||
| "bunyan": "1.8.12", | |||
| "bunyan-gke-stackdriver": "0.1.2", | |||
There was a problem hiding this comment.
Doesn't this couple the service tightly with GCloud? Validators may want to host this on AWS/Azure
There was a problem hiding this comment.
bunyan-gke-stackdriver by @mcortesi is a plugin to bunyan to create an alternatively formatted output stream, it does not require any GCP specific functionality to build/run, and can be configured with LOG_FORMAT=stackdriver, so is opt-in
Codecov Report
@@ Coverage Diff @@
## master #1767 +/- ##
========================================
Coverage 74.21% 74.21%
========================================
Files 278 278
Lines 7652 7652
Branches 673 957 +284
========================================
Hits 5679 5679
Misses 1856 1856
Partials 117 117
Continue to review full report at Codecov.
|
|
Tests pass, but enabled SSH |
Description
This PR adds structured logging and Prometheus events to the attestation service.
Tested
namoffchainrevealRelated issues