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

Addition of New Command for Date Entry Navigation #199

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Shivaji-Dhepale
Copy link

This PR introduces goto date entry navigation to the Ringlogger viewer command system:

Command string can be as follows:

  1. Ringlogger.viewer("d 2025-12-11 10:20:11")

Addition of New Command for Date Entry Navigation:
A new command has been added to facilitate easy navigation to the date entry section. This improves the overall user experience when working with time-sensitive data.

Testing:

Unit Test cases are updated for these enhancements
Conducted manual testing to validate the functionality of the newly introduced command, ensuring it work as expected in various scenarios.
Confirmed that the new date entry navigation command accurately redirects to the date entry section, enhancing usability for time-sensitive operations

Demo:

Command
FAQM-1584-goto-date-command

Response
FAQM-1584-goto-date-test-preview

@@ -130,15 +158,23 @@ defmodule RingLogger.Viewer do
end

defp compute_prompt(state) do
prefix = "[#{state.current_page}/#{state.last_page}] "
prefix =
if state.applications_filter[:start_time] == nil do
Copy link
Member

Choose a reason for hiding this comment

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

Please add a timestamp_filter rather than reusing the applications_filter. That way the two features can be combined.

Comment on lines 165 to 168
{:ok, dt} =
DateTime.from_unix(div(state.applications_filter[:start_time], @microsecond_factor))

"[#{state.current_page}(#{dt})/#{state.last_page}] "
Copy link
Member

Choose a reason for hiding this comment

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

Check out how the prompt is constructed below. It should look something like:

timestamp_suffix = 
  if state.timestamp_filter do
     {:ok, dt} =
        DateTime.from_unix(div(state.applications_filter[:start_time], @microsecond_factor))

    "@#{dt}"
  end

Then add timestamp_suffix to the prompt construction string at the end of the function.

@@ -241,15 +277,19 @@ defmodule RingLogger.Viewer do

defp apply_log_filters(entries, state) do
Enum.filter(entries, fn entry ->
maybe_apply_app_filter?(state, entry) and maybe_apply_level_filter?(state, entry) and
maybe_apply_app_or_time_filter?(state, entry) and maybe_apply_level_filter?(state, entry) and
Copy link
Member

Choose a reason for hiding this comment

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

Add maybe_apply_timestamp_filter?/2 here rather than combining with the app filter.

@@ -434,6 +507,7 @@ defmodule RingLogger.Viewer do
"\t(g)rep [regex/string] - regex/string search expression, leaving argument blank clears filter.\n",
"\t(l)evel [log_level] - filter to specified level (or higher), leaving level blank clears the filter.\n",
"\t(a)pp [atom] - adds/remove an atom from the 'application' metadata filter, leaving argument blank clears filter.\n",
"\t(d)ate (goto date)command - d 2024-12-25 10:20:01 \n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"\t(d)ate (goto date)command - d 2024-12-25 10:20:01 \n",
"\t(d)ate [yyyy-mm-dd hh:mm:ss] - show entries at the specified time.\n",

I see that the code only shows entries at the specified time stamp plus 1 second. I think it would be useful to allow for other durations. Additionally, supporting ISO8601 input directly would be convenient for copy/pasting timestamps.

How about supporting the format:

[start_time] [duration]

Where start_time is specified in a way that can be parsed by .from_iso8601 functions:

[yyyy-mm-dd] - Start of the day (Date.from_iso8601 parses these)
[yyyy-mm-dd hh:mm:ss]
[yyyy-mm-ddThh:mm:ss]
[yyyy-mm-ddThh:mm:ssZ] - NaiveDateTime.from_iso8601 should parse all of these

And duration is specified the ISO8601 way as well. See https://hexdocs.pm/elixir/Duration.html#from_iso8601/1. Unfortunately, the Duration module was only added to Elixir 1.17 and RingLogger supports older versions. Supporting Elixir 1.16 isn't important to me, so I'm fine with not supporting the duration option for earlier versions. That seems easier than maintaining an ISO8601 duration parser into the code base.

If the duration part isn't specified, I was initially expecting it to default to show all log messages after the specified timestamp. I do see that you chose just to show timestamps at the specified second. Do you find that more useful?


@Shivaji-Dhepale
Copy link
Author

Hi @fhunleth , addressed your review comments. Please let us know if anything else required.

@Shivaji-Dhepale
Copy link
Author

Hi @fhunleth ,
As requested, I've created a separate PR for the date entry navigation command.
Additionally, following your suggestion, I’ve provided ISO8601-compliant strings for start_time and duration for Elixir 1.17 and above.
We have ensured that the Duration module loads first before calling any other related functions in case of Elixir lower versions(below 1.17).

@Shivaji-Dhepale
Copy link
Author

Hello @fhunleth,
We're excited to hear your suggestions and look forward to your input!

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

Successfully merging this pull request may close these issues.

2 participants