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

Option to hide timestamps on console logs page #6820

Open
wants to merge 1 commit into
base: jamesnk/consolelogs-commands
Choose a base branch
from

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Nov 27, 2024

Description

Fixes #5457

Demo:

consolelogs-dates

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK changed the title Option to hide dates on console logs page Option to hide timestamps on console logs page Nov 27, 2024
@samsp-msft
Copy link
Member

Cut the date, but keep the time. The dashboard is not typically used to inspect data from long time periods ago, but understanding the delta and frequency of the log messages based on time is important. Showing a long date isn't needed.

@maddymontaquila
Copy link
Member

Cut the date, but keep the time. The dashboard is not typically used to inspect data from long time periods ago, but understanding the delta and frequency of the log messages based on time is important. Showing a long date isn't needed.

Ya agree with @samsp-msft here.

However this is implemented should play nicely with what we decide to do in the future if we start saving dashboard UI state for a local user - ie, resizing/hiding/adding columns, details window pane size

@JamesNK
Copy link
Member Author

JamesNK commented Nov 27, 2024

Today the timestamp matches the ISO 8601 standard. All developers will recognize what it is. I hesitate to change it to just the time, because then it's no longer immediately recognisable as a timestamp.

I also don't want to overload the user with choice. I think the best choices are simple on and off, which means show all (full timestamp) or nothing.

@adamint
Copy link
Member

adamint commented Nov 27, 2024

This looks good to me, but a request if possible would be to add a command to toggle UTC/local time

@@ -73,6 +73,10 @@ private sealed class ConsoleLogsSubscription
[Parameter]
public string? ResourceName { get; set; }

[Parameter]
[SupplyParameterFromQuery(Name = "hideLogDate")]
public bool? HideLogDate { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be non-nullable since the default value is false?

PageViewModel.HideLogDate = true;
await this.AfterViewModelChangedAsync(_contentLayout, waitToApplyMobileChange: false);
},
Text = "Hide log dates",
Copy link
Member

Choose a reason for hiding this comment

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

add to resources

@maddymontaquila
Copy link
Member

Today the timestamp matches the ISO 8601 standard. All developers will recognize what it is. I hesitate to change it to just the time, because then it's no longer immediately recognisable as a timestamp.

I also don't want to overload the user with choice. I think the best choices are simple on and off, which means show all (full timestamp) or nothing.

This makes sense. I'd keep it on by default then, but add the option to change it. I think most people are going to expect timestamps like a regular logfile

@JamesNK
Copy link
Member Author

JamesNK commented Nov 27, 2024

The experience here isn't a log file though, it's console output. A default console doesn't include a timestamp with every line. e.g. Windows command prompt or Windows terminal don't include a timestamp.

@maddymontaquila
Copy link
Member

Sure - but we call them "console logs" on the dashboard and I feel like they're spiritually logs. I'm willing to bend if you and others (@leslierichardson95 mainly) feel strongly that it's worth changing the default behavior though

@drewnoakes
Copy link
Member

My vote would be to keep timestamps by default. They add a lot of information when looking at logs to understand system behaviour, and if users don't find the option to turn them on then they'll be missing out on that info.

I agree with @samsp-msft that the date could be hidden by default though. I don't think a value like 12:34:56.123 is confusing in a log view. It's pretty obviously a timestamp. The full date could be in a tool tip.

await this.AfterViewModelChangedAsync(_contentLayout, waitToApplyMobileChange: false);
},
Text = "Show log dates",
Icon = new Icons.Regular.Size16.Eye()
Copy link
Member

Choose a reason for hiding this comment

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

Rather than change the icon by state, can we use a latched/checked icon state instead? Or maybe we just need a better icon. We use an eye icon for masking secrets throughout.

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

Successfully merging this pull request may close these issues.

5 participants