Skip to content

Conversation

@adamint
Copy link
Member

@adamint adamint commented Mar 26, 2025

Description

In the telemetry filter menu, adds checkboxes for each filter like in console logs settings. Only enabled filters are counted in the badge. Enable/disable state is persisted in the the serialized filter.
image

Used a pause/resume button for the filter dialog (other icon suggestions welcome)
image

Fixes #4143

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 (partially covered by existing tests)
  • 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?

@adamint adamint requested review from JamesNK and Copilot and removed request for Copilot March 26, 2025 20:32
@JamesNK
Copy link
Member

JamesNK commented Mar 27, 2025

UI feedback:

The tick/empty box icons aren't appropriate here because you can't click on the menu item to toggle. What about play and pause as icons to indicate status instead?
Edit: Instead of play icon, how about the same icon as running from the resources page (although standard purple instead of green).
image

Do you think a pause all/enable all option in the menu would be useful?

@JamesNK
Copy link
Member

JamesNK commented Mar 27, 2025

The button position look a little sloppy. What do you think of this:

image

@adamint
Copy link
Member Author

adamint commented Mar 27, 2025

UI feedback:

The tick/empty box icons aren't appropriate here because you can't click on the menu item to toggle. What about play and pause as icons to indicate status instead? Edit: Instead of play icon, how about the same icon as running from the resources page (although standard purple instead of green). image

Do you think a pause all/enable all option in the menu would be useful?

Changed the icons. I do like the pause all/enable all and added that. Let me know if you prefer the filled checkmark circle.

image

@adamint
Copy link
Member Author

adamint commented Mar 27, 2025

The button position look a little sloppy. What do you think of this:

image

image

Changed.

…change icons, move buttons, add disable/enable all
{
<div slot="end">
<FluentCounterBadge HorizontalPosition="70" Count="@(ViewModel.Filters.Count)" Appearance="Appearance.Accent">
<FluentCounterBadge HorizontalPosition="70" Count="@(ViewModel.Filters.Count(filter => filter.Enabled))" Appearance="Appearance.Accent">
Copy link
Member

Choose a reason for hiding this comment

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

There are a lot of filter => filter.Enabled in this code. What about adding EnabledFilters property to the view model, e.g.

public IEnumerable<...> GetEnabledFilters() => Filters.Count(filter => filter.Enabled);

Copy link
Member Author

Choose a reason for hiding this comment

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

We would need to add this to the like 5 view models that contain a filter list. How about this instead?

public static IEnumerable<TelemetryFilter> GetEnabledFilters(this IEnumerable<TelemetryFilter> filters)
{
  return filters.Where(filter => filter.Enabled);
}

Copy link
Member

Choose a reason for hiding this comment

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

Sure

@JamesNK
Copy link
Member

JamesNK commented Mar 27, 2025

Looks good. Some clean up feedback.

@adamint adamint requested a review from JamesNK March 29, 2025 03:32
@JamesNK JamesNK merged commit 550014f into dotnet:main Mar 29, 2025
172 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Idea] Enable/Disable Structured Log Filters on Aspire Dashboard

2 participants