Fixes #1252, Added Comments for Logger Discussion #1782

Metropass · 2021-02-18T17:28:23Z

Issue This PR Addresses

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

So, I wanted to make this PR to discuss what needs to be changed for the Backend's logger.
I've done an audit and added comments to the code on what I personally think needs to be changed, and why.
I would really like some feedback/discussions so that what we change will be useful, and won't omit any data when debugging Telescope.

shutdown.js
There seems to be a lot of error functions being invoked when the service is shutdown. We parse the error into the logger, and mention that the shutdown was not graceful & dirty. Is it possible to move this into debug?
processor.js
-Inside, we have a lot of warnings being sent to the logger. I think that the info calls are appropriate, but should we change the warnings into debug, and return a generic message for production?

On Line 135, we send some data into info mentioning that a certain feed as been skipped. It might be redundant to print this all the time, as the larger the ignored feed list grows, the more messages this will output. Should we change this into debug?
Line 110, we only check for an instance of ArticleError, to let out service continue. Should we add a debug call to this, for developers to see what type of error is thrown?

These were the log calls that I think should be changed, I'm a little bit iffy on changing any calls within backend/web, as they're appropriate (in terms of logging any errors that occur).

I would love to get feedback on what needs to be changed.

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

HyperTHD · 2021-02-18T17:41:30Z

Move all your comments to the side instead of directly below the catch to avoid all these linting errors

Metropass · 2021-02-18T17:42:33Z

Move all your comments to the side instead of directly below the catch to avoid all these linting errors

I'm aware there's some errors, I'm not trying to pass the tests atm until we figure out what to change

yuanLeeMidori · 2021-02-18T23:16:13Z

@Metropass These are my thoughts,

showdown
I agree it is not that necessary for error, maybe warn?
processor
I'm not sure about this, the not-found or error log should be different from the general ones?
line 135
I agree with you, skipping feeds might be more useful in debug level instead of info
line 110
adding it to debug sounds good

I'm still looking for possibility if there is any error log can be moved to critical and warning. I'll append them in this PR if I have progress.

humphd · 2021-02-18T23:42:02Z

src/backend/feed/processor.js

@@ -107,6 +107,7 @@ function articlesToPosts(articles, feed) {
    articles.map(async (article) => {
      try {
        await Post.createFromArticle(article, feed);
+        //Add a debug log to see what is thrown?


We end up logging this in the caller on line 214, and that one could probably be turned into a warning vs. error.

humphd · 2021-02-18T23:45:26Z

src/backend/feed/processor.js

@@ -132,10 +133,12 @@ module.exports = async function processor(job) {
  let info;
  const [invalid, delayed] = await Promise.all([feed.isInvalid(), feed.isDelayed()]);
  if (invalid) {
+    //Debug?
    logger.info(`Skipping resource at ${feed.url}. Feed previously marked invalid`);


I think info is right here, since we'd want to see skipped feeds in production.

humphd · 2021-02-18T23:45:39Z

src/backend/feed/processor.js

    logger.info(`Skipping resource at ${feed.url}. Feed previously marked invalid`);
    return;
  }
  if (delayed) {
+    //Debug?
    logger.info(`Skipping resource at ${feed.url}. Feed previously marked for delayed processing`);


Same, leave this one info

humphd · 2021-02-18T23:46:18Z

src/backend/feed/processor.js

@@ -144,6 +147,7 @@ module.exports = async function processor(job) {
    // If we get no new version info, there's nothing left to do.
    if (!info.shouldDownload) {
      // Log some common cases we see, with a general message if none of these
+      //A lot of warnings & info, should we change it?


These are all useful for debugging, I'd leave them.

humphd · 2021-02-18T23:46:34Z

src/backend/feed/worker.js

@@ -33,6 +33,7 @@ exports.start = async function () {
    logger.info('Connected to elasticsearch!');

    const concurrency = getFeedWorkersCount();
+    //Change into debug?


Yeah, debug, agree.

humphd · 2021-02-18T23:46:45Z

src/backend/lib/shutdown.js

@@ -11,6 +11,7 @@ async function stopQueue() {
    await feedQueue.close();
    logger.info('Feed queue shut down.');
  } catch (error) {
+    //Change to debug?


Agree, debug here.

humphd · 2021-02-18T23:46:56Z

src/backend/lib/shutdown.js

@@ -24,6 +25,7 @@ async function stopWebServer() {
    await serverClose();
    logger.info('Web server shut down.');
  } catch (error) {
+    // Change to debug?


Yeah, debug for sure.

humphd · 2021-02-18T23:47:05Z

src/backend/lib/shutdown.js

@@ -33,6 +35,7 @@ async function cleanShutdown() {
    await Promise.all([stopQueue(), stopWebServer()]);
    logger.info('Completing shut down.');
  } catch (error) {
+    //Change to debug? Shutsdown, but dirty


Agree, debug.

humphd

You've got both branches in here, please separate this work from the other.

chrispinkney · 2021-02-19T17:05:13Z

package.json

@@ -82,6 +82,7 @@
    "body-parser": "1.19.0",
    "bull": "3.20.1",
    "bull-board": "1.3.1",
+    "clean-whitespace": "0.1.2",


What's the point of this? Ah, from the merge?

Yup, it's cleaned out now :)

chrispinkney

Lookin' good!

Metropass self-assigned this Feb 18, 2021

vercel bot deployed to Preview February 18, 2021 17:28 View deployment

humphd requested changes Feb 18, 2021

View reviewed changes

vercel bot deployed to Preview February 19, 2021 15:15 View deployment

vercel bot deployed to Preview February 19, 2021 15:26 View deployment

Metropass force-pushed the log-mop branch from 3791daf to 5f63149 Compare February 19, 2021 15:32

humphd reviewed Feb 19, 2021

View reviewed changes

vercel bot deployed to Preview February 19, 2021 16:48 View deployment

chrispinkney reviewed Feb 19, 2021

View reviewed changes

vercel bot deployed to Preview February 19, 2021 17:13 View deployment

chrispinkney previously approved these changes Feb 19, 2021

View reviewed changes

humphd previously approved these changes Feb 19, 2021

View reviewed changes

humphd added this to the 1.7 Release milestone Feb 19, 2021

Added Comments for Logger Discussion

0c6a99d

birtony dismissed stale reviews from humphd and chrispinkney via 0c6a99d February 19, 2021 19:24

birtony force-pushed the log-mop branch from b403938 to 0c6a99d Compare February 19, 2021 19:24

vercel bot deployed to Preview February 19, 2021 19:24 View deployment

yuanLeeMidori approved these changes Feb 19, 2021

View reviewed changes

PedroFonsecaDEV approved these changes Feb 19, 2021

View reviewed changes

tonyvugithub approved these changes Feb 19, 2021

View reviewed changes

HyperTHD approved these changes Feb 19, 2021

View reviewed changes

birtony approved these changes Feb 19, 2021

View reviewed changes

Metropass merged commit 1210a60 into Seneca-CDOT:master Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1252, Added Comments for Logger Discussion #1782

Fixes #1252, Added Comments for Logger Discussion #1782

Metropass commented Feb 18, 2021 •

edited

Loading

HyperTHD commented Feb 18, 2021

Metropass commented Feb 18, 2021

yuanLeeMidori commented Feb 18, 2021

humphd Feb 18, 2021

humphd Feb 18, 2021

humphd Feb 18, 2021

humphd Feb 18, 2021

humphd Feb 18, 2021

humphd Feb 18, 2021

humphd Feb 18, 2021

humphd Feb 18, 2021

humphd left a comment

chrispinkney Feb 19, 2021

Metropass Feb 19, 2021

chrispinkney left a comment

Fixes #1252, Added Comments for Logger Discussion #1782

Fixes #1252, Added Comments for Logger Discussion #1782

Conversation

Metropass commented Feb 18, 2021 • edited Loading

Issue This PR Addresses

Type of Change

Description

Checklist

HyperTHD commented Feb 18, 2021

Metropass commented Feb 18, 2021

yuanLeeMidori commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispinkney left a comment

Choose a reason for hiding this comment

Metropass commented Feb 18, 2021 •

edited

Loading