feat: add loggers package #1472
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/q9on1kr7d |
There was a problem hiding this comment.
Thanks. I really like that we open source more and more of our internal infrastructure code. Code which is duplicated across services and repositories and quite hard to maintain/keep in sync at time. A problem definitely worthwhile solving. I am just wondering if open sourcing it here right now is the right approach given my level of information. I feel like we are rushing this maybe a bit while having fun on late nite PRs 🤓. I would need us to think maybe a bit more about what we open source and why. What API patterns and guidelines we follow for sets of middlewares here. Especially as they should all feel similar to consumers and what would actually help custom app builders and what verified need we solve. All of which I didn’t have time to think about much but would like to discuss first also with you and stakeholders. As such, at least to my current state of mind, this helps us internally more than anybody else. Lets maybe discuss this soon in person.
|
|
||
| function createErrorReportLogger(options: LoggerOptions = {}) { | ||
| // Sentry | ||
| if (options.sentry) { |
There was a problem hiding this comment.
Hm. Personally I find it rather specific that we use Sentry. Do we know if others using this package would too or should we allow for more reporting targets?
There was a problem hiding this comment.
In my defense, the package says "Opinionated JSON loggers" 😜
emmenko
force-pushed
the
nm-feat-loggers
branch
from
2553032
to
02b1a5e
Compare
|
I understand your points, let's have a chat then 🙂 |
| import * as env from './env'; | ||
| import jsonFormatter from './custom-formats/json'; | ||
|
|
||
| const shouldLog = !env.isTest || process.env.DEBUG === 'true'; |
There was a problem hiding this comment.
This we should maybe extract to a { silent } argument to the createApplicationLogger factory. Others may log under test.
|
|
||
| import * as Sentry from '@sentry/node'; | ||
| import * as env from './env'; | ||
| import redactInsecureRequestHeaders from './utils/redact-insecure-request-headers'; |
There was a problem hiding this comment.
I would not open source this. Maybe we can have a mapHeaders or filterHeaders as a config option (name TBD 😄).
There was a problem hiding this comment.
PS: one of the intents with this loggers package was to provide preconfigured loggers, meaning that for common cases there should be no need to provide extra configuration.
We could maybe have a default list of "insecure" headers, while still allowing to extend that list.
| errorMessageBlacklist?: Array<string | RegExp>; | ||
| }; | ||
|
|
||
| function createErrorReportLogger(options: LoggerOptions = {}) { |
There was a problem hiding this comment.
Maybe calling it createSentryErrorMiddleware is more apt.
| delete info.level; | ||
|
|
||
| // Replace / rename fields | ||
| replaceField(info, 'meta.req.query', 'meta.req.queryJsonString'); |
There was a problem hiding this comment.
Should be an option omitMetaPaths: String[] I think.
There was a problem hiding this comment.
Yeah, probably we need to provide some kind of fields mapping.
{
rewriteMetaInfoFields: {
'meta.req.query': 'meta.req.queryJsonString'
}
}An alternative might be to allow to pass a custom logform formatter. We can also expose a bunch of default formatter (e.g. json, kibana, etc). The kibana formatter can also be additionally configured with the fields rewriter.
There was a problem hiding this comment.
Yeah. I think sane defaults which can be overwritten would be good. Maybe also by just saying: I want a Kibana logger as a type argument you get those defaults.
| return req[propName]; | ||
| }; | ||
|
|
||
| const createAccessLogger = (options: LoggerOptions = {}) => { |
There was a problem hiding this comment.
This is a middleware I think. So to have it inline with others maybe it's createAccessLoggerMiddleware(options).
packages-backend/loggers/src/env.ts
Outdated
| const isProd = env === 'production'; | ||
| const isTest = env === 'test'; | ||
|
|
||
| export { isDev, isProd, isTest }; |
There was a problem hiding this comment.
I would also have this as an arg to the middlewars. envGetters maybe. Not 100% also just making notes for myself.
|
I cleaned things up and removed all unnecessary things. I also removed Sentry, as I think we can keep this internal for now. @tdeekens can you take another look? |
|
|
||
| // Re-export winston for convenience | ||
| import * as winston from 'winston'; | ||
| export { winston }; |
There was a problem hiding this comment.
Nit: doesn't TS allow export * as winston from 'winston'`?
Partially part of #1444
This PR open sources our internal loggers package, used by our backend services.