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

Exporting invocation order of Filters #15237

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

saumya1singh
Copy link
Contributor

@saumya1singh saumya1singh commented Feb 22, 2021

Fixes: #14885

@geoand
Copy link
Contributor

geoand commented Feb 22, 2021

Thanks!

It looks like it already conflicts with what we have on master :)

Moreover, I'll need to take a closer look to let you know what needs to be done to progress to meet the actual needs mentioned in #14885

@saumya1singh
Copy link
Contributor Author

saumya1singh commented Feb 22, 2021

It looks like it already conflicts with what we have on master :)

Yeah, I see the conflict is because of the previous PR I made.
I was working on DevConsoleProcessor before as well. That is my code only, which I wrote for Static Resources Task :)

@saumya1singh saumya1singh marked this pull request as draft February 22, 2021 16:02
@geoand
Copy link
Contributor

geoand commented Feb 23, 2021

This is a good start, but it's not exactly what we are looking for to solve #14885.
The general idea is for a user to be able to see which filters are associated with which endpoint.

Looking at the existing DevUI, I think we should add that information into the scores page (the UI will need some attention as to not cluter each entry).

To determine the filters for each endpoint you will need to update EndpointScore to contain a list of request filter entries. Those entries will a new static class inside the ScoreSystem class that looks like so:

    public static class RequestFilterEntry {
        public final String name;
        public final boolean preMatch;

        public RequestFilterEntry(String name, boolean preMatch) {
            this.name = name;
            this.preMatch = preMatch;
        }
    }

Those filter entries will be determined in this method by doing something like:

            ServerRestHandler[] handlerChain = runtimeResource.getHandlerChain();
            List<RequestFilterEntry> requestFilters = new ArrayList<>();
            for (ServerRestHandler serverRestHandler : handlerChain) {
                if (serverRestHandler instanceof ResourceRequestFilterHandler) {
                    ResourceRequestFilterHandler requestFilterHandler = (ResourceRequestFilterHandler) serverRestHandler;
                    requestFilters.add(new RequestFilterEntry(requestFilterHandler.getFilter().getClass().getName(), requestFilterHandler.isPreMatch()));
                }
            }

Note: To make that work, you'll also need to update ResourceRequestFilterHandler to add a couple getters"

    public ContainerRequestFilter getFilter() {
        return filter;
    }

    public boolean isPreMatch() {
        return preMatch;
    }

Once you do all that, each EndpointScore will contain the list of request filters which you need to display in nice manner in the UI.

Does this all make sense?

@saumya1singh
Copy link
Contributor Author

Yes, this is all very well explained and it is now clear: going to start the work :)

@geoand
Copy link
Contributor

geoand commented Feb 23, 2021

Cool :)

@saumya1singh
Copy link
Contributor Author

So now I am trying to expose all the details of requestFilters from DevConsoleProcessor into my filters.html.
But I don't understand this piece of an error on running mvn quarkus:dev.

image

Perhaps this is related to my code :)?

@geoand
Copy link
Contributor

geoand commented Feb 23, 2021

I would suggest doing a clean build of Quarkus and of the project you are using to test it.
Furthermore, for the project you are testing, make sure that you are using the same version of Quarkus for both the quarkus-bom and the quarkus-maven-plugin

@saumya1singh
Copy link
Contributor Author

oh nice, it worked :)
I should have asked early: I wasted time fixing on my own 🤦🏻

@geoand
Copy link
Contributor

geoand commented Feb 23, 2021

A rule of thumb is that anytime you see a NoSuchMethodError, there is a version mismatch somewhere :)

@saumya1singh
Copy link
Contributor Author

saumya1singh commented Feb 23, 2021

Is it a good way to show Filters like this?
Corresponding to each filter should we show the diagnostic.score as well?

image
image

@geoand
Copy link
Contributor

geoand commented Feb 23, 2021

A filter does not have a score.

I was thinking that filters should be in some kind of UI component that allows you to open / close them. The reason is that there could potentially be a lot of filters and in that case that would really mess up the UI.

@saumya1singh
Copy link
Contributor Author

Humm like for one single endpoint, one can have a huge list of Filters.

yes that definitely makes sense to put it in a UI component that allows to open/close them else the score page will be flooded with list of filters.

@geoand
Copy link
Contributor

geoand commented Feb 23, 2021

Humm like for one single endpoint, one can have a huge list of Filters.

Yeah, there is no telling how many there could be.
Zero is very likely, but so is something like 5.

@saumya1singh
Copy link
Contributor Author

filterListBadgeSecondary.mp4

After trying diff styles, ended here: well I am working to improve this too :)
Something like this will perhaps do the work?

@geoand
Copy link
Contributor

geoand commented Feb 24, 2021

Yeah, that is a step in the proper direction.

Can you also add a simple screenshot pointing to the part in question? I would like @phillip-kruger to be able to see it and perhaps provide suggestions for alternatives :)

@saumya1singh
Copy link
Contributor Author

Can you also add a simple screenshot pointing to the part in question? I would like @phillip-kruger to be able to see it and perhaps provide suggestions for alternatives :)

Sure, I was thinking to put this on zulip too so that can get better feedback and suggestions from everyone :)

@geoand
Copy link
Contributor

geoand commented Feb 24, 2021

Sure, although I think that Phillip's advice is all you'll need :)

@saumya1singh
Copy link
Contributor Author

saumya1singh commented Feb 24, 2021

@phillip-kruger

I added the Filters part as pointed in the screenshot. On clicking Click to view Filters List, we are able to see the list of Filters :)
image
image

I am thinking of using a better UI component 🤔 WDYT

@phillip-kruger
Copy link
Member

phillip-kruger commented Feb 24, 2021

Hi @SaumyaSingh1 - I think what you have is great already. Here some notes on the screen and what I would possibly change:

As I understand, the main goal of the screen is to show the score of the endpoints, first at a global value and expanded in these 3 categories:

  • Execution
  • Resource
  • Writer

Then as a second goal provide some metadata about the endpoints, like

  • filters
  • content type

I think users might 1) have many endpoints and 2) really mostly care about the endpoints that does not score high, right ?

So how about we order the endpoints from low scoring to high scoring, so that the "bad" ones is at the top. Then maybe use an
Accordion (https://getbootstrap.com/docs/4.6/components/collapse/#accordion-example) that only list the Endpoints and their total score, and if selected open up with a section that contain the 3 detailed scores.

Then, what would be cool is if those 3 scored can be more visible, so I was thinking, maybe a basic Gauge per score ?

Then at the bottom a table with the other meta data on the endpoint, with the filters using a collapse (https://getbootstrap.com/docs/4.6/components/collapse/) like you have now, but maybe just in icon to activate the "show"

So something like this maybe :

idea

On another note: I would rename the page name and remove the "Quarkus REST" part, and just call it Scoring (we know it's Quarkus and we know it's Rest :) )

Let me know if I can help you with this :)

@phillip-kruger
Copy link
Member

Here a pure CSS Gauge example: https://github.com/JohnrBell/Gauge_CSS

@geoand
Copy link
Contributor

geoand commented Feb 24, 2021

WOW, that design is awesome!

@phillip-kruger
Copy link
Member

Thanks @geoand :) It does not have to be gauges, just somehow make the score more visible, as at the moment , metadata and score blur a bit and it's a bit of an alignment exercise to see what score is where.

@saumya1singh
Copy link
Contributor Author

Awesome, this design idea is soo beautiful and clean 💙 @phillip-kruger

Definitely, I would love to do the redesigning of the score page :) But I think for that I will need to open a separate PR because this one is mainly focused on the "Filters" Category. WDYT @geoand

As Phillip suggested for the Filters I will keep the simple > kinda icon (simple and nice) and of-course using collapse :)

@geoand
Copy link
Contributor

geoand commented Feb 24, 2021

Definitely, I would love to do the redesigning of the score page :) But I think for that I will need to open a separate PR because this one is mainly focused on the "Filters" Category. WDYT @geoand

Yes, for sure

@phillip-kruger
Copy link
Member

What about adding the "Meta data" table at the bottom ? The we can later work on the score section ?

@geoand
Copy link
Contributor

geoand commented Feb 24, 2021

If @SaumyaSingh1 is up for it or if you want to chip in sure, why not :)

@saumya1singh
Copy link
Contributor Author

sure :)

DEV UI : Adding Filters in Score Page

added initial filter information

adding code

adding code- not final

DEV UI: adding filters

Committing Final Code for Filters
@saumya1singh saumya1singh marked this pull request as ready for review February 25, 2021 05:14
@saumya1singh saumya1singh changed the title WIP : Exporting invocation order of Filters Exporting invocation order of Filters Feb 25, 2021
@geoand
Copy link
Contributor

geoand commented Feb 25, 2021

Can you post a screenshot of what the page looks like now?

@saumya1singh
Copy link
Contributor Author

Sure, it is simple right now with the Filter functionality added :)

image
image

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGMT

@geoand geoand closed this Mar 1, 2021
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Mar 1, 2021
@geoand geoand reopened this Mar 1, 2021
@geoand
Copy link
Contributor

geoand commented Mar 1, 2021

I closed and re-opened to get CI working

@saumya1singh
Copy link
Contributor Author

saumya1singh commented Mar 1, 2021

nice, all checks passed :)

@geoand geoand merged commit 895ae89 into quarkusio:master Mar 1, 2021
@quarkus-bot quarkus-bot bot removed the triage/invalid This doesn't seem right label Mar 1, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Mar 1, 2021
@geoand
Copy link
Contributor

geoand commented Mar 1, 2021

Thanks @SaumyaSingh1 !

@geoand geoand mentioned this pull request Mar 8, 2021
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.

Export invocation order
3 participants