-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Faceted search on the jobs list page #5236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋
Cool, I love the way you've done a separate freestyle example first for this and then integrated. I had a little play with it and left some inline comments for the moment. Before I look a little more though it would be good to get some info on some things here.
-
I noticed lots of references to
qp
,QP
,QueryParam
,queryParam
. If I was working on this, I feel that I would be often asking myself 'is this variable/method using the initials or words, is it capitialized?' Overall, do you need to use this prefix here so much? I know you probably won't ever do this, but what if you decided to store the filters elsewhere for some reason, like in localStorage, or just not store them at all? -
What was the decision to use
facet
instead offilter
based on? I think this kind of thing is generally called a filter, so 'Filter this list based on datacenter and status for me'. I keep thinking 'what's a facet?' whilst I'm reading this. I'm also looking at the templates and thinking maybe just using:
options=types
selection=currentType
...
options=statuses
selection=currentStatus
or maybe
options=type.options
selection=type.current
...
options=status.options
selection=status.current
Would be good to see what you think here, and try to understand more where you are coming from regarding all of the above?
Other things I noticed:
-
I initially thought the prefix filtering was broken as it was only showing my one prefix out of a couple in the table. It was only when I looked at the code I saw you only put them in the prefix filter select if there is more that one instance of that prefix. Do you think a user of this may wonder if something is broken?
-
For the prefix idea itself, what about people using
:
or something else? Is the_-.
thing a common nomad semi-standard?
Anyway, come back to me when you can, I've not approved as yet as I'd like to understand the queries above first. I'm guessing there no immediate rush with this, but let me know if so. Feel free to shout me offline if thats easier.
Thanks,
John
return []; | ||
} | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe I see what you mean here by unattractive! So my URL right now has:
?status=%5B"pending"%2C"running"%2C"dead"%5D
Generally I've seen these things done with status[]=thing
, I'm not sure how ember treats that though, I did notice something the other day related to this, I'll try and dig it out again in a bit incase it helps.
I'm not entirely sure that "
should be in URLs, and also that ,
shouldn't - probably worth having a look to see what is 'usual' here. Do you need the "
's? Are you expecting to have true
and "true"
at the same time?
Anyway, if you take a look lemme know what you find out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the thing I was looking at the other day which is ever so slightly related to this
Looks like ember is aware of the key[]=value
thing, just be careful as I'm not sure it encodes keys properly when you do that. I don't think that would effect you here anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was honestly expecting Ember to use the status[]=&status[]=
style when I bound an array to a query param, but that's not what happened. Instead it basically does what I'm doing now: JSON stringifying.
As for encoding characters, it's curious that you are seeing "
and ,
in the URL, what browser are you using? In Chrome, they were automatically encoded. Not sure if that's a property of Ember or Chrome.
Do you need the "'s? Are you expecting to have true and "true" at the same time?
Yes-ish. Ideally I would change qpSerialize
to just arr.join(',')
, but commas (and basically everything) is a legal job name and datacenter character. So there are other possible encoding schemes than JSON.stringify
, but it's not worth spinning my wheels on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for encoding characters, it's curious that you are seeing
"
and,
in the URL
Oh weird were you not getting "
s? I was in Chrome. I did find it strange that it seemed to be encoding one but not the other. I'm gonna check this branch out again in a bit and double check. It was definitely showing me actual "
s when I did this first pass.
but commas (and basically everything) is a legal job name and datacenter character
Yeah for sure, this is why I was thinking a filter[status][]=url%20encoded%2Cjob-name&filter[status][]=another%22url%20encoded%2Cjob-name
seemed to make more sense to me? I'm not entirely sure what ember does though, so I'm not sure how easily it is to make ember produce/consume that - I might have a bit more of a dig in a bit myself later.
Instead it basically does what I'm doing now: JSON stringifying.
So my question would be, what's the reason for doing it here if ember does the same? I don't have enough practical info on what ember does here though.
I think I'd still be tempted to use URL encoding to encode strings for the URL - but there's also an argument to say 'use the framework', and if it uses a javascript encoder to encode the entire data blob into a single string parameter and then URL encode it then maybe there is a reason for that I'm not aware of/haven't thought of. It might just be 'because it's more straightforward'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hist[prefix] = hist[prefix] ? hist[prefix] + 1 : 1; | ||
} | ||
return hist; | ||
}, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm rubbish at RegEx so not sure on this one, but is there a way to test and match the prefix in one shot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The regex could be changed to capture up to the delimiter and also require characters after the delimiter. Then if there's no match or the capture group is empty, that'd mean there was no prefix.
I'm not sure how large of an impact that would make on performance here, but I suppose it's worth investigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. It wasn't really the performance angle that I was coming from (maybe it's more performant to use a test
to potentially avoid a match
), I don't think performance is important here.
I think it was more of a 'tool for the job' thing.
Actually, if anything from a performance angle, I'd hoist your regex definitions to the top scope maybe? I don't think it matters here though really, I don't think this will ever be that hot, but I could be wrong, don't have enough context there.
|
||
return true; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, I'd consider using some curly brackets instead of one line conditionals here, especially at the end here it gets a little "hold on, what's happening here". I never use this type of one line conditionals though so maybe its just me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's definitely dense, but a bunch of curlies seems noisy. I had to try really hard to avoid overengineering this 😄 I'll poke at it some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, this is more verbose but to my eyes its easier to read so thanks for that! I realized after my comment that your original code might have been more readable originally but maybe prettier
wrapped it to make it 'prettier'? 🤷♂️
I think this is more just a readability/syntax/style thing rather than an engineering thing, you could maybe do the engineering differently, but I don't think it matters too much here (see below)
Just a last note here if it helps, I use a switch
approach with things like this quite a lot, means you get the benefit of one-liners but readability is maintained, again maybe just a 'to my eyes' thing:
switch(true) {
case types.length && !types.includes(job.get('displayType')):
case statuses.length && !statuses.includes(job.get('status')):
case datacenters.length && !job.get('datacenters').find(dc => datacenters.includes(dc)):
case prefixes.length && !prefixes.find(prefix => job.get('name').startsWith(prefix))
return false;
default:
return true;
}
Comments can fit in really nicely between the case
statements there also if you ever need to get more wordy with what's happening. I generally put the most likely thing to happen at the top.
Little engineering offshoot, but why all the .length
checks? Is that to ensure things aren't null
/undefined
. includes
and find
will return a falsey value on zero length arrays if I'm not mistaken?
Hi @DingoEatingFuzz ! Are you planning to add the Unsynced status, as found in the Services view of the Hashi UI ? This status is very helpful to us to detect production problems. |
Hello @nanoz, the In Sync/Unsynced status in Hashi UI relates to consul service health, right? The plan is to do a full consul integration, which will include health checks in the job row as well as an additional facet in this filtering view so you can filter jobs by their consul health. |
To me, this status is not Consul related. It shows there are differences between what we expect (the job definition) and what actually is (the current state of the job). |
Okay, I reread through the hashi-ui code and I think I figured it out this time. A job is "in sync" when its both a service and all its expected allocations are running. If this is the case, it seems like the underlying use case here is to find jobs that are "stuck"? Jobs that have allocations that are perpetually queued for one reason or another. If this is the case, there is no current plan to build this, but I am interested in the use case. Simply basing synchronicity on running allocations isn't a complete solution due to deployments, especially with canaries, but potentially down the road we could better query something like "jobs with allocations that have been queued for longer than X time". |
Hi @johncowen finally getting around to addressing your questions.
I took inventory of all the places I use qp/QP/queryParam
So, definitely a variety of naming schemes. I think that can be tidied up.
If I wasn't using query params, I'd likely do a lot of this code differently. All of the
This type of search is called faceted search. These are also filters, but lots of things are filters. IMO, facet is a more descriptive name.
I like that first option. My concern that led to long names is that there is a lot going on in the controller, so it might be clearer if I gave the selections and options specific names. Maybe that's not necessary. The second option looks nice and grouping like-properties in objects is of course good, but it makes specifying dependent keys and setting properties harder.
Maybe? It's definitely an experiment that may fall short, but if the goal here is to help people filter jobs, I don't want to introduce the problem of having to weed through all the prefixes. I don't think a prefix used on one job is worth filtering by. Maybe I'm wrong thinking that.
I just chose them because they are common semi-standards across all naming conventions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋
Thanks for revisiting this! I've had a quick look over your inlines and added some more thoughts. I'm going to try check this back out and have another deeper look now and come back later.
This type of search is called faceted search. These are also filters, but lots of things are filters. IMO, facet is a more descriptive name.
Oh cool, never heard that, I looked it up also:
https://en.wikipedia.org/wiki/Faceted_search
It does sound like this fits with what you are doing here so fair enough, I'd still wonder how many people would recognise setFacetQueryParam
over setFilterQueryParam
, but again possibly just a me thing so don't worry.
Hopefully if this doesn't work for people, they will file issues.
Cool, sounds good - I like the MVP and then iterate thing. Do you folks ever do any sort of pre-release user testing? We've got some really good detailed info from users that might help with things like this?
Hopefully come back with some more thought here or a LGTM before you are about today
Cheers,
John
return []; | ||
} | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for encoding characters, it's curious that you are seeing
"
and,
in the URL
Oh weird were you not getting "
s? I was in Chrome. I did find it strange that it seemed to be encoding one but not the other. I'm gonna check this branch out again in a bit and double check. It was definitely showing me actual "
s when I did this first pass.
but commas (and basically everything) is a legal job name and datacenter character
Yeah for sure, this is why I was thinking a filter[status][]=url%20encoded%2Cjob-name&filter[status][]=another%22url%20encoded%2Cjob-name
seemed to make more sense to me? I'm not entirely sure what ember does though, so I'm not sure how easily it is to make ember produce/consume that - I might have a bit more of a dig in a bit myself later.
Instead it basically does what I'm doing now: JSON stringifying.
So my question would be, what's the reason for doing it here if ember does the same? I don't have enough practical info on what ember does here though.
I think I'd still be tempted to use URL encoding to encode strings for the URL - but there's also an argument to say 'use the framework', and if it uses a javascript encoder to encode the entire data blob into a single string parameter and then URL encode it then maybe there is a reason for that I'm not aware of/haven't thought of. It might just be 'because it's more straightforward'
hist[prefix] = hist[prefix] ? hist[prefix] + 1 : 1; | ||
} | ||
return hist; | ||
}, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. It wasn't really the performance angle that I was coming from (maybe it's more performant to use a test
to potentially avoid a match
), I don't think performance is important here.
I think it was more of a 'tool for the job' thing.
Actually, if anything from a performance angle, I'd hoist your regex definitions to the top scope maybe? I don't think it matters here though really, I don't think this will ever be that hot, but I could be wrong, don't have enough context there.
|
||
return true; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, this is more verbose but to my eyes its easier to read so thanks for that! I realized after my comment that your original code might have been more readable originally but maybe prettier
wrapped it to make it 'prettier'? 🤷♂️
I think this is more just a readability/syntax/style thing rather than an engineering thing, you could maybe do the engineering differently, but I don't think it matters too much here (see below)
Just a last note here if it helps, I use a switch
approach with things like this quite a lot, means you get the benefit of one-liners but readability is maintained, again maybe just a 'to my eyes' thing:
switch(true) {
case types.length && !types.includes(job.get('displayType')):
case statuses.length && !statuses.includes(job.get('status')):
case datacenters.length && !job.get('datacenters').find(dc => datacenters.includes(dc)):
case prefixes.length && !prefixes.find(prefix => job.get('name').startsWith(prefix))
return false;
default:
return true;
}
Comments can fit in really nicely between the case
statements there also if you ever need to get more wordy with what's happening. I generally put the most likely thing to happen at the top.
Little engineering offshoot, but why all the .length
checks? Is that to ensure things aren't null
/undefined
. includes
and find
will return a falsey value on zero length arrays if I'm not mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
I've had another play with this in place and I have a few non-important right now comments re: usability more than anything, plus I added a couple more things I included inline.
I think overall some of it doesn't feel like your usual tidy style (mainly re the comment inline on moving some vars to the top of your class definition). So maybe take another pass at that? Maybe you have a reason for doing it different here?
I had a few other things that aren't hugely important right in the scheme of things, and I want to make sure things can keep moving on this, so I'm going to approve as everything works/tests pass etc. Just I think there are refinements that could optionally be made at some point, but you may get to some of these or plan some of these before you finally merge all this into master. I'll list some of these here just FYI but I'm not suggesting these absolutely need to be done.
-
There is a lot of UI 'jumping around'. Clicking on things in the filter makes widths of the table change, widths of the filter buttons change, even the URL does this strange changing thing (it seems to swap the order of the query params after a second or so). All this jumping around can get a bit finickity for example you can click on a filter under your mouse to tick a checkbox and the menu moves out from under you mouse so you can't immediately click again to untick the checkbox, you have to move your mouse again and then click.
-
How useful are the numbers that appear in your filter buttons? How useful is it to know that I've filtered by 3 or 4 filters? Or do I just need to know that it contains an enabled filter? It's a funny one this as we've been discussing this in a semi-related thing we have over in consul land. Would a count/total of jobs with that type be a more useful number here? This might be especially useful in a paged UI as it would make it easier to know at a glance maybe if you had any 'dead' jobs, and how many.
-
I know you did a great job on the mobile menu and you put a lot of effort into make the UI accessible. I did a quick squeeze of this to check what the mobile view was like. Are there any plans for responsive layouts here for people wanting to do a quick check for say 'dead' jobs with a quick glance on a mobile?
-
I'm guessing you will want to reuse a lot of this code for other models/areas (so allocations etc). I can think of a few things here that could make this difficult, so I'm interested to see what your plans are/the outcome is for doing this.
Lastly a little question for me for the future. What's the easiest way to turn off all the mirage console.logging whilst looking at this. I was scattering some console.logs about myself whilst looking at this and the mirage logging makes it difficult to see any other logs that I'm adding.
Anyway ping me if you want to talk further about any of this.
ui/app/controllers/jobs/index.js
Outdated
selectionStatus: qpSelection('qpStatus'), | ||
selectionDatacenter: qpSelection('qpDatacenter'), | ||
selectionPrefix: qpSelection('qpPrefix'), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all these go up at the top of the class definition with your other variables?
/** | ||
Filtered jobs are those that match the selected namespace and aren't children | ||
of periodic or parameterized jobs. | ||
*/ | ||
filteredJobs: computed('model.[]', '[email protected]', function() { | ||
visibleJobs: computed('model.[]', '[email protected]', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are changing the name of the property here, maybe change the comment also? Or maybe the comment refers to the code under this computedProperty now? Not sure
scheduleOnce('actions', () => { | ||
this.set( | ||
'qpDatacenter', | ||
qpSerialize(intersection(availableDatacenters, this.get('selectionDatacenter'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use a.filter(item => b.includes(item))
here? Will save you a dependency? Not sure if I've read this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd work fine, but it wouldn't save me a dependency since I'm already using intersection
elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah cool, I think it's just here:
nomad/ui/app/models/allocation.js
Line 73 in c12bdcd
return intersection(taskGroupUnhealthyDrivers, nodeUnhealthyDrivers); |
You could remove lodash there also? I suppose it depends what side of the fence you are on, but it would save a dependency, no biggie either way.
Thanks for the review John, this is definitely still a WIP regarding the big picture of faceted search. As for your four points, here are my responses.
|
🙏 thanks for giving me your reasoning here, I think a lot of it makes sense. Onwards and upwards! |
Changelog says this was released on 0.9.2, I run 0.9.5 but don't see these features. Do I need some special flag to enable it? |
@Fuco1, there is no special flag. I just tested with 0.9.2 and see the filters as long as a job is running. My only suggestions are to clear your local cache, and verify that the server nodes are in fact running 0.9.5. Wish I had a better response for you 🙁 |
The servers are running 0.9.5 according to the server tab I cleared the local storage in my browser and did a hard-reload and a browser restart.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This is part two of three for faceted search in the Nomad UI: #5235
Faceted search on the Jobs page introduces four quick filter options for finding jobs.
-
,_
, or.
)Facet selections are persisted in query params in order to be shareable and bookmarkable.