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
Implement Redwood API side Logger #1937
Conversation
First main questions:
|
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 added a couple of thoughts about the code.
Setting this up in an existing app was a breeze! Only thing that I didn't see work is the Prisma logging, but I believe this is not yet implemented, right?
That's correct @jvanbaarsen - I am not sure how best to simplify the setup. This is my current // See https://www.prisma.io/docs/reference/tools-and-interfaces/prisma-client/constructor
// for options.
import { PrismaClient } from '@prisma/client'
import { logger } from './logger'
/* Determines the type and level of logging.
*
* See: https://www.prisma.io/docs/reference/api-reference/prisma-client-reference#log
*/
const defaultPrismaLoggerOptions = [
{
emit: 'event',
level: 'query',
},
{
emit: 'event',
level: 'info',
},
{
emit: 'event',
level: 'error',
},
{
emit: 'event',
level: 'warn',
},
]
export const db = new PrismaClient({
log: defaultPrismaLoggerOptions,
errorFormat: 'colorless',
})
/*
* To use event-based logging:
* Set emit to event for a specific log level, such as query
* Use the $on() method to subscribe to the event
*
* https://www.prisma.io/docs/concepts/components/prisma-client/working-with-prismaclient/logging
*/
db.$on('query', (queryEvent) => {
logger.debug(
{ ...queryEvent },
`${queryEvent.query} in ${queryEvent.duration} msec`
)
})
db.$on('info', (logEvent) => {
logger.info({ ...logEvent }, logEvent.message)
})
db.$on('warn', (logEvent) => {
logger.warn({ ...logEvent }, logEvent.message)
})
db.$on('error', (logEvent) => {
logger.error({ ...logEvent }, logEvent.message)
}) I'm not sure how best to include this:
{ query: Boolean, levels: ['info, 'warn']}
|
Thanks! That was my goal. The pino logger is easy, but wanted to make the DX somewhat "out-of-the box".
I spent some time this morning and converted my app's // See https://www.prisma.io/docs/reference/tools-and-interfaces/prisma-client/constructor
// for options.
import { PrismaClient, Prisma } from '@prisma/client'
import { logger } from './logger'
/* Determines the type and level of logging.
*
* See: https://www.prisma.io/docs/reference/api-reference/prisma-client-reference#log
*/
const logLevels: Prisma.LogLevel[] = ['info', 'warn', 'error'] // add 'query' in dev?
const prismaLoggerOptions = (
logLevels: Prisma.LogLevel[]
): Prisma.LogDefinition[] => {
return logLevels?.map((level) => {
return { emit: 'event', level } as Prisma.LogDefinition
})
}
const createPrismaClient = (logLevels: Prisma.LogLevel[]) => {
const client = new PrismaClient({
log: prismaLoggerOptions(logLevels),
errorFormat: 'colorless',
})
/*
* To use event-based logging:
* Set emit to event for a specific log level, such as query
* Use the $on() method to subscribe to the event
*
* https://www.prisma.io/docs/concepts/components/prisma-client/working-with-prismaclient/logging
*/
logLevels.forEach((level) => {
if (level === 'query') {
client.$on(level, (queryEvent: Prisma.QueryEvent) => {
logger.debug(
{ ...queryEvent },
`${queryEvent.query} in ${queryEvent.duration} msec`
)
})
} else {
client.$on(level, (logEvent: Prisma.LogEvent) => {
logger.info({ ...logEvent }, logEvent.message)
})
}
})
return client
}
export const db = createPrismaClient(logLevels) This still creates the db instance but sets up all the logging in Prisma based on just the level chosen:
@jvanbaarsen Can you try this out to see if you like it? If so perhaps change the CRWA db.js template to something like this. One idea would be to construct:
based on the logger's level and |
Actually, what is very cool i that one can warn on slow queries (ie duration over a threshold): logLevels.forEach((level) => {
if (level === 'query') {
client.$on(level, (queryEvent: Prisma.QueryEvent) => {
if (queryEvent.duration > 20) {
logger.warn(
{ ...queryEvent },
`Slow Query performed in ${queryEvent.duration} msec`
)
} else {
logger.debug(
{ ...queryEvent },
`Query performed in ${queryEvent.duration} msec`
)
}
})
} else {
client.$on(level, (logEvent: Prisma.LogEvent) => {
logger.info({ ...logEvent }, logEvent.message)
})
}
}) then it appears as:
|
Note: I've added Moving out of draft. Subsequent PRs will setup logFlare and dataddog transports. |
@dthyresson I'll give the Prisma setup a try tomorrow 👍 |
export const logger = createLogger({ | ||
options: { ...defaultLoggerOptions }, | ||
}) |
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.
Maybe we can just leave these as the default in the function itself?
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 thought about that, but I really like explicitly showing it here because you gives some nice help:
and
which I think help you know what the defaults are and the expected config out-of-the-box.
What do you think, @peterp ? Happy to move though.
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.
Ah some typos ... I fix!
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.
You can actually make that part of the type, then it'll be auto-completed and typed checked. I think the comment will also show up.
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.
Would you be OK with making the defaultLoggerOptions
part of the function, and using types to document them to the user during design time?
If so, and if you need any guidance to make this happen let me know.
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.
Would you be OK with making the defaultLoggerOptions part of the function,
I think so.
if you need any guidance to make this happen let me know.
I'm not sure I understand what is needed to do that though.
export const createLogger = ({
options,
destination,
showConfig = isDevelopment,
}: RedwoodLoggerOptions): BaseLogger => {
Include the defaultLoggerOptions
always? And then just override with options
that are LoggerOptions
?
Like
export const createLogger = ({
options,
destination,
showConfig = isDevelopment,
}: RedwoodLoggerOptions): BaseLogger => {
const loggerOptions = { ...defaultLoggerOptions, options }
and then use loggerOptions
in the function?
That way you'd declare a logger like:
export const logger = createLogger()
But if you do want to override:
export const logger = createLogger({ options: { level: 'warn' } })
?
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.
Looking great David, I've left a few notes, questions and suggestions
export const createPrismaClient = (setLogLevels?: Prisma.LogLevel[]) => { | ||
const client = new PrismaClient({ | ||
log: prismaLoggerOptions(setLogLevels), | ||
errorFormat: 'colorless', | ||
}) | ||
|
||
setLogLevels && configureLogLevels(client, setLogLevels) | ||
|
||
return client | ||
} |
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.
@dthyresson I know this has now merged, but I was wondering if we could hide most of this implementation?
The reason is I'm worried that Prisma changes the logging interfaces and we have to ship a huge code mod.
Maybe there's a way to internalize configureLogLevels
?
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.
Definitely would like to.
I commented yesterday above:
Can we hide these implementations in the logger library in the api package?
@peterp I didn't know if I wanted to have the api/logger depend on Prisma.
Some deploys won't have Prisma so then would that prevent people from using the logger?
I'm concerned that Prisma changes the format
...
But not sure I can think of an alternative.
But of course when you say:
The reason is I'm worried that Prisma changes the logging interfaces and we have to ship a huge code mod
I definitely want to avoid that scenario.
I am having a hard time visualizing what
if we could hide most of this implementation?
or
internalizing
it might look like?
Can you give me a direction or maybe we jump of a quick call?
I wanted to get logging in so people could give feedback like this and can improve the implementation so can assess if we want to ship the CRWA template changes in w/ v0.28.
…r-rwt-link * 'main' of github.com:redwoodjs/redwood: (26 commits) Router: Fix auth (redwoodjs#2038) Steps towards a11y for Redwood Router (redwoodjs#1817) Router: <Set wrap={}> (redwoodjs#1898) Pass event and context into getCurrentUser (redwoodjs#1908) Implement Redwood API side Logger (redwoodjs#1937) Fixed path on windows to allow for pages under subdirectories (redwoodjs#2022) Add experimental ESBuild for api side (redwoodjs#1948) Upgrade to Prisma 2.19.0 (Prisma Migrate GA) (redwoodjs#2021) Fix lint breaking when deleting a side (redwoodjs#2017) Refactor: Converted Prisma.ts to js (redwoodjs#1958) Fix issue with verify email redirect using Auth0 (redwoodjs#1990) add GitHub Action CodeQL Analysis (redwoodjs#1951) fix: correct var name for grabbing user config (redwoodjs#2002) Create functions to fs calls (redwoodjs#2007) Return signup Output (redwoodjs#1992) Add makeExecSchema options (redwoodjs#1964) upgrade gotrue-js to 0.9.29 (redwoodjs#2011) Azure Active Directory Auth: Adding try-catch block on callback to capture empty key (redwoodjs#2010) withCellHOC: Fix TS error (redwoodjs#1967) Update error message in tasks/publish-local to point to tasks/run-local-npm when Verdaccio isn't running (redwoodjs#2004) ...
Implements api-side logging using the pino logger.
This is an opinionated logger that has some useful default configuration, such as:
This feature
/src/lib/logger.ts
to the CRWA template (lives alongside db.js)Example
/src/lib/logger.ts
and use:then ...
Please see:
Still TODO
context.logger.debug({model}, 'My message'}
For example, this would auto add the function path to the log info data which could be useful in custom functions
Ideally, I would want to fetch the resolver name and set it.
Some Example Configs for Different Loggers
These examples are in the CRWA logger.ts file.
Prisma
I'd like to make it easy to log Prisma db output as well.
I'd like to simplify or include this setup: