File transport: lazy file creation #1507
Conversation
| @@ -105,6 +108,10 @@ module.exports = class File extends TransportStream { | |||
| return true; | |||
| } | |||
|
|
|||
| if (this.lazy) { | |||
| this.open(); | |||
There was a problem hiding this comment.
I worry whether this affects performance because this gets called on every log() call -- we should check and see
There was a problem hiding this comment.
Right! The first thing open does when it's called, it's checking if it was already opened, but it's an overhead of extra function call in every log operation. So, I will try to change this to flag check here and measure performance.
| @@ -105,6 +108,10 @@ module.exports = class File extends TransportStream { | |||
| return true; | |||
| } | |||
|
|
|||
| if (this.lazy) { | |||
| this.open(); | |||
There was a problem hiding this comment.
How does this work with e.g. file rotation? Maybe it's fine, but I think it would be good if we could add one more test case that makes sure file rotation still works as expected with lazy: true.
There was a problem hiding this comment.
Yes indeed, current tests of file rotation don't have this covered. I will add some tests.
There was a problem hiding this comment.
Yeah, the lazy option messes up with rotation and stress tests. I think that it's because they are assuming immediate file creation (assertions are constructed that way), not that the outcome is invalid.
|
Thank you so much for looking into this! I left a few comments for your consideration. |
|
Thanks for the review! I will work on that. |
|
Closing this for now due to inactivity but please feel free to re-open anytime. |
This change tries to address the issue #1494.