jekyll-archives icon indicating copy to clipboard operation
jekyll-archives copied to clipboard

Add debug logging to assist in troubleshooting

Open kriation opened this issue 5 years ago • 18 comments

Recently, I was struggling to understand why archives was generating duplicate pages. Without debug logging in how archives was processing each set of pages, it was difficult to determine where the issue was.

I added the code in this PR to assist and hope to close issue #145

kriation avatar Nov 16 '19 15:11 kriation

@kriation Instead of leaving the opening comment of a Pull Request empty and opening a related issue-ticket, it is better if the PR's raison d'être is contained within the PR itself.

However, if you're submitting a PR to resolve an existing issue ticket, you can include Closes <Issue No.> in the opening component.

ashmaroli avatar Nov 18 '19 09:11 ashmaroli

I don't have a sample site at hand to test this manually. Can you please post screenshots of the debug output from this change? Thanks.

ashmaroli avatar Nov 18 '19 09:11 ashmaroli

Can you please post screenshots of the debug output from this change? - @ashmaroli

Attached! 2019-11-18_14:35:08

kriation avatar Nov 18 '19 19:11 kriation

If this is is accepted, this should be documented on the README.

DirtyF avatar Nov 19 '19 13:11 DirtyF

There are couple of things immediately apparent from the screenshot:

  • This adds additional noise to an already noisy verbose output. :roll_eyes: Imagine what the output would be for a site with hundreds of posts..
  • categories should be debugged as with category: #{title} not tag:
  • Jekyll.logger.debug has been called with just a single parameter instead of two (missing comma between the two strings..)

All in all, I'm not in favor of this proposal. Sorry.

ashmaroli avatar Nov 19 '19 13:11 ashmaroli

@ashmaroli IIUC this will only be output when you use --debug flag, not by default right?

DirtyF avatar Nov 19 '19 13:11 DirtyF

will only be output when you use --debug flag

Yes, that is correct. I was thinking maybe instead of piping into the default debug output, it'd be better if these output was triggered via an additional configuration... For example,

jekyll-archives:
  debug: true

But that needs to be approved by other maintainers first.

ashmaroli avatar Nov 19 '19 14:11 ashmaroli

@ashmaroli - I'm happy to make the changes you mentioned.

With regard to noisy verbose output, isn't verbosity verbose by design?

Without this logic, resolving issues (as I did) would take considerably longer as there are no other mechanisms to debug what is being processed in this plugin.

kriation avatar Nov 19 '19 14:11 kriation

noisy verbose output, isn't verbosity verbose by design? ... ...no other mechanisms to debug what is being processed

Unfortunately, true, to both questions. Perhaps you can use a private helper to iterate through the list of posts and log each post to the debug output..

resolving issues (as I did)

What exactly was that issue?

ashmaroli avatar Nov 19 '19 14:11 ashmaroli

Perhaps you can use a private helper to iterate through the list of posts and log each post to the debug output..

How is this different than using the debug logger?

What exactly was that issue?

Nuance in how I specified tags in post front matter.

kriation avatar Nov 19 '19 14:11 kriation

How is this different than using the debug logger?

Greater control and flexibility.. For example:

def read_tags
  if enabled? "tags"
    tags.each do |title, posts|
      debug_key(posts, title, "tag")
      @archives << Archive.new(@site, title, "tag", posts)
    end
  end
end

private

def debug_key(posts, title, key)
  posts.each do |post|
    debug "Processing #{post.relative_path} with #{key}: #{title}"
  end
end

def debug_date(...)
  ...
end

def debug(msg)
  return unless site.config.dig("jekyll_archives", "debug")
  Jekyll.logger.debug "Archives:", msg
end

Disclaimer: The above example needs to be optimized further..

ashmaroli avatar Nov 19 '19 14:11 ashmaroli

Understood! I'll update my branch with the changes.

kriation avatar Nov 19 '19 15:11 kriation

I'll update my branch with the changes.

I was hoping for some inputs or approval of that idea from one of the other maintainers before you made additional changes.. Either ways, the config key for this plugin is jekyll-archives instead of jekyll_archives in my example. ref: https://github.com/jekyll/jekyll-archives/blob/master/docs/configuration.md

ashmaroli avatar Nov 19 '19 15:11 ashmaroli

The debug output should only be displayed when using the --debug option

@DirtyF That's how it is already. Jekyll.logger.debug cannot output in default level (:info)

ashmaroli avatar Jan 28 '20 02:01 ashmaroli

@DirtyF What is your opinion on resolving RuboCop offenses on this branch by using the example in https://github.com/jekyll/jekyll-archives/pull/144#issuecomment-555530872 as a guide..?

ashmaroli avatar Jan 29 '20 16:01 ashmaroli

fine by me

DirtyF avatar Jan 29 '20 17:01 DirtyF

@kriation Thank you for bring this PR up to date. But my requests for changes still remains.. I'll approve if you're able to refactor this to meet the following:

  • The proposed debug output is an opt-in — users have to explicitly enable {"jekyll-archives"=>{ "debug"=>true}} via their config file.
  • Have the above documented in the docs/configuration.md file.
  • Extract the debugging logic into a private method that prints each archive entry in a new line. For example:
          Archives: Processing documents with the tag: 'foobar'
                    _posts/2018-05-06-hello-world.markdown
                    _posts/2018-05-08-hello-again.markdown
                    _posts/2018-05-12-the-truth-is-out-there.markdown
                    _posts/2019-06-07-hold-the-door.markdown
                    _posts/2019-07-14-truth-justice-and-stuff.markdown
                    _posts/2020-07-04-static-is-the-new-black.markdown
          Archives: Processing documents with the category: 'lipsum'
                    _posts/2018-05-06-hello-world.markdown
                    _posts/2018-05-08-hello-again.markdown
                    _posts/2018-05-12-the-truth-is-out-there.markdown
                    _posts/2019-06-07-hold-the-door.markdown
                    _posts/2019-07-14-truth-justice-and-stuff.markdown
                    _posts/2020-07-04-static-is-the-new-black.markdown
          Archives: Processing documents in the year: '2019'
                    _posts/2019-06-07-hold-the-door.markdown
                    _posts/2019-07-14-truth-justice-and-stuff.markdown
          Archives: Processing documents in the month: '2018/05'
                    _posts/2018-05-06-hello-world.markdown
                    _posts/2018-05-08-hello-again.markdown
                    _posts/2018-05-12-the-truth-is-out-there.markdown
          Archives: Processing documents from the day: '2018/05/06'
                    _posts/2018-05-06-hello-world.markdown

ashmaroli avatar Jul 05 '20 11:07 ashmaroli

@ashmaroli Absolutely! I noticed earlier this week that the original commits were well behind the existing mainline, so this last commit was to bring it up to par. I'll take a stab at implementing your changes and will add it to the commit list for this PR.

kriation avatar Jul 07 '20 19:07 kriation