Closed dthyresson closed 2 years ago
@dthyresson Ideally this would be upgraded before v1.0.0-rc. Keep me posted!
Following on https://github.com/redwoodjs/redwoodjs.com/pull/760
I'd appreciate your thoughts how one might change the
destination
option to support multiple ones, too.
I've been testing a few things and it appears one can use multistreams without changing RW:
export const logger = createLogger({
options: { prettyPrint: true },
destination: pino.multistream(
[
{ level: 'debug', stream: HB() },
{ level: 'error', stream: HB2() },
],
{ levels: 'log' }
),
})
That, for me, has worked. Problem is however that pino.multistream
is not really recommended because there are performance concerns. If we do blend this into RW's logger, I think we should at least fire a console warning if we add up to a certain limit of streams.
Now the problem with pino.multistream
is that it is executed in the main thread of the pino instance, hence a resulting overload if we add too many streams.
From what I gather it would be best to implement the pino.transport
with multiple targets solution, but this one is harder to get working. On the pro side, we get separate threads ( for each transport I believe ), on the other side we'll have to work on loaders to get this one working. Indeed, for a transport configuration, the property options.destination
is nothing like the pino.destination
prop we know, it allows to output to a file but hardly more.
So we should rely on the prop target
, per doc:
target: The transport to pass logs through. This may be an installed module name, an absolute path or a built-in transport
Couldn't get this to work with a module name, built-in transports are something different, so I went the absolute path road and it becomes a mess.
There's one last thing for me to check before declaring a roadblock on the pino.transport
solution, which is if its options.destination
cannot actually take pino.destination()
as a valid parameter.
Edit: not working, moving on...
If it's the case I'd suppose we should move forward with pino.transport
, if not, hopefully someone can get it to work, or we'll have to consider an implementation with pino.multistreams
.
From what I gather it would be best to implement the pino.transport with multiple targets solution,
Right -- 7.x Pino introduces this pattern:
const pino = require('pino')
const transport = pino.transport({
targets: [
{ target: '/absolute/path/to/my-transport.mjs', level: 'error' },
{ target: 'some-file-transport', options: { destination: '/dev/null' }
]
})
pino(transport)
Where "targets" are multiple transports
Currently, RedwodLoggerOptions
are:
/**
* RedwoodLoggerOptions defines custom logger options that extend those available in LoggerOptions
* and can define a destination like a file or other supported pin log transport stream
*
* @typedef {Object} RedwoodLoggerOptions
* @extends LoggerOptions
* @property {options} LoggerOptions - options define how to log
* @property {string | DestinationStream} destination - destination defines where to log
* @property {boolean} showConfig - Display logger configuration on initialization
*/
export interface RedwoodLoggerOptions {
options?: LoggerOptions
destination?: string | DestinationStream
showConfig?: boolean
}
with a single destination. Should we:
If it's the case I'd suppose we should move forward with pino.transport
Right, so we can use that and just add one or multiple destination stream, perhaps.
To me looking quickly, seems like instead of destination
we add targets
and it passes into Pino with same structure it expects.
Happy to have you give it a try.
Hey @dthyresson
Just a heads up, I'm still interested in the task but we are under pressure at SportOffice to begin our first live trials during current season - therefore, I won't be available for it until September.
I'd be keen on keeping destination
for backward compatibility and targets
for multiple transports, as it will reflect on pino
's API. At least supporting both for a while until a single destination would be migrated to the history of logging culture.
Also, not sure it's in the doc, but should we end up with both targets
and destination
, I'd update the doc recommending to at least set up a solid destination
stream as some sort of safeguard, while demonstrating how targets
can make it more interesting. Any thoughts on this?
Lastly, if we move forward with keeping both, shall we make them mutually exclusive in favor of transports
?
Hey @noire-munich and @dthyresson I learned a lot about Pino today, just because I want to make it work with Logtail 🥲 Now that I understand how with V6, I think I should work on directly with Pino V7.
I also think we should go only with transports
, but all the supports and documentation is not ready yet, so it will be harder to start.
I have open a draft PR # 3298 just to start
@thedavidprice Since @simoncrypta completed the Pino v7 upgrade for the single destination, can we downgrade this issue to post v1.0 to support multiple transports. Multiple transports require workers and not sure how to do that in a server-less way yet. And is not a core need at the moment for 1.0.
I moved this to hopper 🦘
Yes, understood. And that's the right label! I'm going to do the same with the PR. Thanks.
To continue the conversation I had with @dthyresson , the next step to remove the deprecated prettyPrint
option is :
options: { prettyPrint: 'something' }
from doc and ts interface.overwriteDev
(can be something else) to overwrite default log config in dev (useful to test new prod log in dev)export const logger = createLogger({
options: { level: 'info' }
destination: stream,
transports: [{
target: 'pino/file',
options: { destination: '${BASE_DIR}/${LOG_FILENAME}' }
}], // transports (Multithreaded Logging) will overwrite destination (singlethreaded Logging)
overwriteDev: { // this will overwrite default dev log ( prettier to STDOUT )
options: { level: 'info' }
destination: stream,
transports: [{
target: 'pino/file',
options: { destination: '${BASE_DIR}/${LOG_FILENAME}' }
}], // transports (Multithreaded Logging) will overwrite destination (singlethreaded Logging)
}
})
What do you all think about this ?
@simoncrypta I am hoping with the addition of a custom log formatter in dev (see: https://github.com/redwoodjs/redwood/pull/3947) then we can close this out and support the latest Pino with no pretty print warnings.
then we can close this out and support the latest Pino with no pretty print warnings.
Amazing !
@simoncrypta I updated to 7.0.6 in #3947.
Maybe we can close out?
No multiple transports support for the V1, but a fully functional Pino V7. Let's close it 👍
Pino 7.0.0-rc is out.
https://github.com/pinojs/pino/releases/tag/v7.0.0-rc.1
Improvements include making it easier to define multiple transport streams and options.
For example:
Need to see if just upgrading is enough -- or if what to make the Logger api support multiple destinations.
The
destinations
would then declare multiple transport targets.