Rotating log file for #1534

[Kiduzk]

An implementation for #1534

[nuki-chan]

Lines 6-15

Ohayou, esteemed developer-san! Nuki has detected your intentions to improve logging, and while the effort is commendable, there are a few nuances that could make this even better! Let's tighten up those logs like we're prepping for the grand anime convention! 🌠

• The pad function is super adorable, doing its own little "I must include leading zeros" dance, but what if I told you we could avoid the dance altogether? JavaScript's String.prototype.padStart is like the cool senpai that's been around the block and has all the answers. Let's use that instead, and Nuki promises you'll still keep the cuteness factor! 🎀
• The check if (!time) is giving Nuki déjà vu vibes because it looks like we're preparing for some time travel! But since we're not in a sci-fi episode, perhaps we should ensure consistency in naming patterns between log files during no-time times and regular-time times, ne? 🚀
• Now, about that nested path building with month folders, Nuki wonders, do we have the code elsewhere to handle the creation of these directories? They don't just pop into existence like a magical girl transformation, after all! Make sure there's logic to handle directory creation if we're segmenting logs this way. ✨

Here's a little code magic from Nuki to help out:

const logNameGenerator = (time, index) => {
  if (!time) return path.join(app.getPath('userData'), 'logs', 'nuclear-error.log');

  const month = time.getFullYear().toString() + (time.getMonth() + 1).toString().padStart(2, '0');
  const day = time.getDate().toString().padStart(2, '0');
  const hour = time.getHours().toString().padStart(2, '0');
  const minute = time.getMinutes().toString().padStart(2, '0');

  return path.join(app.getPath('userData'), 'logs', `${month}/${month}${day}-${hour}${minute}-${index}-nuclear-error.log`);
};

Lines 16-25

Lookie here! We're getting all fancy with rotating log files! 📜 But, there's always room for a little more polish, isn't there? Here are some hints from your code-review kouhai:

• The log rotation size has shrunk from a bountiful '5M' to a mere '1M'. While Nuki admires minimalism, do ensure: "Does this size make sense for our use-case?" Smaller can be cuter, but it should also be practical. 🌀
• The maxFiles property mysteriously appeared like a secret character being introduced late into the season. It's indeed a good practice to limit the number of log files! Good job on that, but let's document why we chose 3 as the magic number, okay? 🧙‍♀️
• Lastly, the compression method 'gzip' will indeed make our logs slimmer, much like a good diet before cosplaying. Yet, I hope we're not compressing prematurely—consider the performance impact and the frequency of access before deciding to compress logs.

Remember, following Nuki's advice is like reading manga -- it's not required but it often leads to enlightenment, and occasionally, a good laugh. 😜 Also, Nuki might make a mistake sometimes, because even bots powered by the latest AI technologies can have bugs (but don't tell anyone, okay?). 🙊

Now, go forth and make these logs spectacular! Ganbatte, developer-chan! 💖✨

[nukeop]

If you use a filename dependent on the current day and time, will this overwrite earlier existing files? My suspicion is that these files will stay there indefinitely, because they will all have different names and the logger won't know which files to overwrite. Do you think you could test that?

[Kiduzk]

Update: I did some testing, and it seems like the module discards the oldest files and adds a new one. This is a screenshot between two successive rotations:

[Kiduzk]

Also, looks like the module creates a separate file to hold the current log. Not quite sure if we would want that or not

[nukeop]

Ok, that behavior looks good. Could you write a test to make sure it stays this way? You could mock fs and assert how the files are created.

[Kiduzk]

I have come to realize that what you said about the file names using date and time is correct. That approach does not work as intended. My apologies, I did not understand properly.

Good thing is I think that we can just add one line argument rotate which seems to do the job. I have committed this change, and tested it manually. How does it sound? Also, if this change sounds good, may I ask guidance on the location to put the testing file? I was thinking to put in same folder with the index file of the logger and write a logger.test.ts there. Thank you for your time and patience!

[nukeop]

Nice, then I propose that I could try testing this manually this evening, and then if everything works as expected I could write the tests for it. I also want to replace electron-timber so I might do it at the same time.

[Kiduzk]

Sounds good!

[nukeop]

I'm struggling to test and debug this library, it's very hard to mock its fs calls correctly. While I have a reasonable test setup, I need to study its internal behavior, and I can't step into its classical function, if I set it up so that there is already a saved log file to check how it rotates, the function doesn't progress through the rest of its rotation logic. I'm aware this is testing a dependency, but I want to be sure that the way it's initialized in Nuclear's logging service is ok and works as expected, and I can't do that.

[nukeop]

Thanks for contributing. Let's discuss a couple of details in the comments

[nukeop]

Because you force-pushed to your branch, I can't push to your branch anymore, because the history is messed up and it's no longer continuous. Not sure how to add the tests I wrote to this PR.

[Kiduzk]

Hey @nukeop I apologize for the hassle I created with the force push. Is there a way we can fix it? Please let me know if you need me to do anything regarding the force push issue or the testing, I am more than willing to help. I would really appreciate your guidance if possible, I am kind of new to opensource stuff

[nukeop]

I tried to do several things and I have no idea how to fix this. I tried pushing to a parallel branch: https://github.com/nukeop/nuclear/tree/Kiduzk-temp
But then I can't merge this branch into your master. Try it yourself, maybe you'll be able to.

In general you should never force push, especially to a branch others might be working on, it destroys git history. It's only allowable in cases where you commit important secrets that can't be rotated (even in those cases it's too late to protect these secrets anyway).

[nukeop]

Btw, the test on that branch is unfinished, and I'm not convinced it's going to be necessary. Maybe I could just merge this PR?

[Kiduzk]

As far as the testing goes, merging works with me. I run some manual tests with small log sizes and the output seems to be as we want it. This is one screenshot I have

[Kiduzk]

Also, I was able to merge your branch with my master branch locally. Would you suggest that I push these changes to my GitHub fork associated with this PR?

[nukeop]

Yep, push them to this branch. If you do, I'll be able to add further commits

[Kiduzk]

Sounds good, I have pushed them

[nukeop]

Now that I pulled this locally, it seems to be fixed. Thanks.

[nukeop]

For some reason I still can't push to your branch. Could you please remove the last test in logger.test.ts and leave everything else? And then we could merge this PR.

[Kiduzk]

Sure, no problem. I have removed the last test, let me know if that works

[nukeop]

Can you remove the last test in that file? You only deleted some mocks. I wanted to do it myself but I still can't push to your branch.

[Kiduzk]

Yea, no problem. How about now

[nukeop]

Hey, sorry I did not merge this yet, but I - literally - cannot:

I tried resolving these conflicts but again, I couldn't pull the changes locally, so I'm stuck. I think a good course of action would be to simply create a brand new branch from master, and manually copy these changes. Then I could merge that branch, and adapt it later manually when it's merged to master.

[Kiduzk]

I guess I learnt lesson to not force push for future, thanks or bearing with me. I can get started on copying to new branch