Skip to content
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

Renderer logging to file #6740

Closed
wants to merge 1 commit into from

Conversation

samitiilikainen
Copy link
Contributor

@samitiilikainen samitiilikainen requested review from Nokel81 and a team December 13, 2022 09:14
@samitiilikainen samitiilikainen requested a review from a team as a code owner December 13, 2022 09:14
@samitiilikainen samitiilikainen requested review from aleksfront and removed request for a team December 13, 2022 09:14
instantiate: (di) => {
const logger = di.inject(loggerInjectable) as winston.Logger;

return logger.add(new transports.File({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that writing logs to filesystem from renderer caused issues in the past. Not sure if it is still the case...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point, do we know if this transport uses async or sync FS calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Winston file transport is using async fs.createWriteStream

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think after #6690 is merged this will be simpler.

Also, I think that we should include the clusterId in the file name so that we don't have multiple frames writing to the same file.

@samitiilikainen samitiilikainen marked this pull request as draft December 14, 2022 06:47
@Nokel81 Nokel81 added this to the 6.3.0 milestone Dec 15, 2022
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants