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

Clear traces/structured logs functionality #6898

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

Conversation

Daluur
Copy link

@Daluur Daluur commented Dec 8, 2024

Description

Added a button to traces and structured pages, to clear all items.
I find it very useful to be able to easily get a clean slate, as it makes it easier to see what is new items. I have worked with other similar tools in the past, and have had a great use of such buttons.

ClearLogs
ClearTraces

The placement and icon of the buttons might not be perfect.
I utilized the existing Clear functionality in the CircularBuffer to implement this change.

This is similar to this reported issue: #5458, just in different pages.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship. However, there might be feedback on the button placement and icon.
    • 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?
    • Yes
    • No (at least I don't think it is needed)
Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 8, 2024
@Daluur
Copy link
Author

Daluur commented Dec 8, 2024

@Daluur please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@JamesNK
Copy link
Member

JamesNK commented Dec 8, 2024

Nice.

I have some UI thoughts:

  • Should there be a confirmation before clearing? i.e. a Yes/No dialog page that pops up and says are you sure you want to clear structured logs/traces. The console logs clear button in Chrome developer tools doesn't, so I lean towards no.
  • The location of the button next to the resource dropdown could be confusing about what you're clearing. If (All) is selected then obviously you're clearing logs for all resources. If a resource is selected then are you still clearing all logs, or just logs for the selected resources?
  • Doesn't need to be in this PR, but we'll want to do the same thing on the metrics page and console logs page. But console logs is more difficult because the data is stored externally. Either we'd need to tell the external source to clear its data, or filter it when it arrives to only logs after the cleared time.

@Daluur
Copy link
Author

Daluur commented Dec 9, 2024

Nice.

I have some UI thoughts:

  • Should there be a confirmation before clearing? i.e. a Yes/No dialog page that pops up and says are you sure you want to clear structured logs/traces. The console logs clear button in Chrome developer tools doesn't, so I lean towards no.
  • The location of the button next to the resource dropdown could be confusing about what you're clearing. If (All) is selected then obviously you're clearing logs for all resources. If a resource is selected then are you still clearing all logs, or just logs for the selected resources?
  • Doesn't need to be in this PR, but we'll want to do the same thing on the metrics page and console logs page. But console logs is more difficult because the data is stored externally. Either we'd need to tell the external source to clear its data, or filter it when it arrives to only logs after the cleared time.
  • I think a pop up would just end up being annoying
  • I agree that the button placement is not ideal, I had a hard time finding a better place within the page
  • Initially I did not think it would be as useful for metrics, but it might be nice to simply clear everything

What if, instead of having a button on every page, there is a single button somewhere, that clears everything; traces, metrics, and structured logs (and console logs at some point)? The button could be located in the top bar next to the settings button, or perhaps inside the settings dialog, if in the header is too prominent.

I have done some work on clearing metrics and clearing traces when viewing trace details, but I have not been able to finish it today, I will likely continue work tomorrow

@Daluur
Copy link
Author

Daluur commented Dec 10, 2024

I made some updates.

Clear in top menu

  • I moved the clear button to the top menu, next to settings
  • I added functionality to clear metrics

The button clears all signal data, and can be extended to clear console logs in the future.
Clearing of metrics does not work 100%. I think it might be the way the data is populated from OLTP, where it might get 'old' data again, once it has been cleared, though, I am not certain. I intend to debug in a few days

@JamesNK
Copy link
Member

JamesNK commented Dec 11, 2024

I'm not sure having it in the top toolbar is an improvement. That area isn't related to the data you see on the page. Also, maybe it is useful to be able to independently clear certain areas of telemetry (and individual resources) rather than a clear all button. Or maybe we have both, i.e. a clear all button in the dashboard settings sidebar and clear buttons on individual telemetry pages.

I think the Aspire team is going to need to talk amoungst ourselves about how we want this feature to work. I'm not sure if that will happen this year - people are disappearing for holidays - but I'll make sure there is a decision before Aspire 9.1 ships, and if you're not available to finish the PR off when we have a decision then I'll make whatever changes are required to get it in.

I think this is valuable. Something that isn't perfect is better than not shipping.

@JamesNK
Copy link
Member

JamesNK commented Dec 12, 2024

I thought about the UI more and talked it over with other Aspire folks offline. Good news is consensus on what we want the UI to look like here:

  • Have a clear button next to the resource filter on each telemetry page (and the console logs page in the future). This is similar to what your PR originally had. However, clicking on it opens a menu with a couple of options:

    • Remove all
    • Remove for resource (if a resource could be selected. If (All) is selected then it is disabled)
    • Maybe, in the future, Remove visible (allows someone to remove an exact filtered set of telemetry)
      This is like what Fiddler has in its UI. I think it is simple but gives lots of flexibility. Also, it is a sneaky way of requiring a confirmation: the user clicks to expand the menu, then clicks to remove. They won't remove data accidently by just clicking on the remove button.
      fiddler-remove
  • Also have a Remove all button in settings sidebar (below where the theme is set). This button would clear all telemetry for all resources in one click. The is like the button you added top menu but moved to settings.

This is more complicated than your original PR, but I think the remove menu on the telemetry pages plus a remove all button in settings handles all scenarios while being intutive.

@JamesNK
Copy link
Member

JamesNK commented Dec 12, 2024

For the remove menu, you can use the AspireMenuButton control. We use that in a couple of places. The actual button itself can be two icons Delete + ChevronDown. It would look would be like what GH has with the new button:

image

If you have trouble with the menu, or logic to remove telemetry for one resource, or don't have time to work on this anymore, let me know and I can help with the PR.

@Daluur
Copy link
Author

Daluur commented Dec 13, 2024

Thanks for the feedback :) I'll get to work on some of the things, I'll let you know if I'll need reinforcements

@Daluur
Copy link
Author

Daluur commented Dec 19, 2024

I have now implemented a dropdown button on the traces and structured logs pages, with the option to either remove all or only the selected resource(s).
I could not figure out how to make the AspireMenuButton use 2 icons, I might just be missing something obvious, for now I just used ChevronDown

Logs-remove-dropdown
Traces-remove-dropdown

For traces, when removing specific resources, it will remove the entire Trace if the root span came from that resource, otherwise it will remove all spans from the resource. I am unsure if it should also remove Span links.

I have not tackled Metrics yet, but I will look into it within too long

@JamesNK
Copy link
Member

JamesNK commented Dec 22, 2024

I’m away for a week. I’ll take a look when I’m back.

I might just be missing something obvious, for now I just used ChevronDown

It’s a limitation of the control. Leave it as is and it can be improved in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants