[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/796 +1 ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/796 + 1. Gret job. all pass login to application â should display error message for invalid credentials â should login for valid credentials â should logout metron-alerts App â should have all the UI elements â should have all pagination controls and they should be working â should have all settings controls and they should be working â play pause should start polling and stop polling â should select columns from table configuration â should have all time-range controls â should have all time range values populated - 1 â should have all time range values populated - 2 â should have all time range values populated - 3 â should have all time range values populated - 4 â should disable date picker when timestamp is present in search â should have now included when to date is empty â should have all time-range included while searching metron-alerts configure table â should select columns from table configuration â should rename columns from table configuration metron-alerts Search â should display all the default values for saved searches â should have all save search controls and they save search should be working â should populate search items when selected on table â should delete search items from search box â should delete first search items from search box having multiple search fields â manually entering search queries to search box and pressing enter key should search metron-alerts tree view â should have all group by elements â drag and drop should change group order â should have group details for single group by â should have group details for multiple group by â should have sort working for group details for multiple sub groups â should have search working for group details for multiple sub groups metron-alerts facets â should display facets data â should expand all facets â should have all facet values â should collapse all facets metron-alerts alert status â should change alert status for multiple alerts to OPEN â should change alert status for multiple alerts to DISMISS â should change alert status for multiple alerts to ESCALATE â should change alert status for multiple alerts to RESOLVE â should change alert status for multiple alerts to OPEN in tree view metron-alerts alert status â should change alert statuses â should add comments for table view â should add comments for tree view Executed 42 of 42 specs SUCCESS in 5 mins 2 secs. [01:01:00] I/launcher - 0 instance(s) of WebDriver still running [01:01:00] I/launcher - chrome #01 passed ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/796 login to application â should display error message for invalid credentials â should login for valid credentials â should logout metron-alerts App â should have all the UI elements â should have all pagination controls and they should be working â should have all settings controls and they should be working â play pause should start polling and stop polling â should select columns from table configuration â sould have all time-range controls â sould have all time-range included while searching - Expected 'Alerts (0)' to equal 'Alerts (5)'. metron-alerts configure table â should select columns from table configuration metron-alerts Search â should display all the default values for saved searches â should have all save search controls and they save search should be working â should populate search items when selected on table â should delete search items from search box â should delete first search items from search box having multiple search fields â manually entering search queries to search box and pressing enter key should search metron-alerts tree view â should have all group by elements â drag and drop should change group order â should have group details for single group by â should have group details for multiple group by â should have sort working for group details for multiple sub groups â should have search working for group details for multiple sub groups metron-alerts facets â should display facets data â should expand all facets â should have all facet values â should collapse all facets metron-alerts alert status â should change alert status for multiple alerts to OPEN â should change alert status for multiple alerts to DISMISS â should change alert status for multiple alerts to ESCALATE â should change alert status for multiple alerts to RESOLVE â should change alert status for multiple alerts to OPEN in tree view - Failed: element not visible (Session info: chrome=61.0.3163.100) (Driver info: chromedriver=2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2),platform=Mac OS X 10.12.6 x86_64) metron-alerts alert status â should change alert statuses â should add comments for table view â should add comments for tree view ** *Failures* ** 1) metron-alerts App sould have all time-range included while searching - Expected 'Alerts (0)' to equal 'Alerts (5)'. 2) metron-alerts alert status should change alert status for multiple alerts to OPEN in tree view - Failed: element not visible (Session info: chrome=61.0.3163.100) (Driver info: chromedriver=2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2),platform=Mac OS X 10.12.6 x86_64) ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/796 I think the test coverage looks pretty good. I'm +1 conditional on the e2e tests passing. ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user iraghumitra commented on the issue: https://github.com/apache/metron/pull/796 @merrimanr I have added e2e tests as suggested and merged your PR too. Plz, let me know if I missed anything. I had to omit assertions for a couple of quick ranges as they can cause trouble when we have leap years and 29 day months. I hope it's not an issue. ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user merrimanr commented on the issue: https://github.com/apache/metron/pull/796 This issue is still not fixed: - The first is related to time-range selections that include 'Now' as part of the range (Last 7 days, Last 5 minutes, Today so far, etc). This should be a sliding window so I would expect the search query to be different every time the results are refreshed. - Fixed I submitted a PR against this PR that should address it (also has the latest version of master merged in). Let me know if you think this is the right way to fix it. Seems kind of strange to import a class (QueryBuilder) in alerts-list into a service but I'll let you decide if that is ideal. Saved search is working now and everything else seems to function correctly. I did notice a regression where if I rename a column, type a query with the friendly name in the query box (ie. sourceType:snort) results are no longer returned. Not sure if that was introduced in this PR or not because I don't think we are testing for it. I am still not a fan of how the Time Range selection works. The "now" designation is essentially meaningless because you can't save a range with "now" in either FROM or TO. I still agree with all my previous complaints about this too. If the community feels it is working correctly then I would not hold up this PR over it. The e2e tests are still insufficient and failing. I gave some recommendations [here](https://github.com/apache/metron/pull/796#issuecomment-337965321) for your reference. ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/796 @iraghumitra looks like everything has been addressed. I am +1 on my side, but lets have @merrimanr chime in ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user iraghumitra commented on the issue: https://github.com/apache/metron/pull/796 @merrimanr @james-sirota There was a bug in handling the recent searches that were in old format can you check it now. Sorry for the trouble ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/796 A few things didn't work for me. First, when I select a time range of (t-x minutes) the start and end time does not fill in per screen shot below. https://user-images.githubusercontent.com/4854764/31926145-b2ac5c52-b841-11e7-9fde-aa9d6ba3267d.png;> The time box was initially populated with the word "now", which did not allow me to save. I had to manually enter valid timestamps in order for this to work: https://user-images.githubusercontent.com/4854764/31926214-0d02a616-b842-11e7-8811-b5dae19f63d4.png;> When I click to save a search I get the following exception: TypeError: path must be absolute or specify root to res.sendFile at ServerResponse.sendFile (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/response.js:410:11) at /usr/metron/0.4.1/web/expressjs/alerts-server.js:71:7 at Layer.handle [as handle_request] (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/layer.js:95:5) at next (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/route.js:137:13) at Route.dispatch (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/route.js:112:3) at Layer.handle [as handle_request] (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/layer.js:95:5) at /usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/index.js:281:22 at param (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/index.js:354:14) at param (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/index.js:365:14) at Function.process_params (/usr/metron/0.4.1/web/expressjs/node_modules/express/lib/router/index.js:410:3) ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user iraghumitra commented on the issue: https://github.com/apache/metron/pull/796 @merrimanr can you clear browser cache and try? There is a change in save query model my bad I missed mentioning it. ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user iraghumitra commented on the issue: https://github.com/apache/metron/pull/796 @merrimanr plz find my replies - when I open the date/time range picker why does it say "now/d"? I would expect just "now" or "Now" - Changed to now - when I first navigate to the Alerts UI or select "All time" why do I see a timestamp clause in the query? Why not just leave it out? - Fixed - The first is related to time-range selections that include 'Now' as part of the range (Last 7 days, Last 5 minutes, Today so far, etc). This should be a sliding window so I would expect the search query to be different every time the results are refreshed. - Fixed - I also found the time range selector value isn't populated correctly when loading a saved or recent search. -Fixed - if I select a cell in the table to apply a filter, then remove the filter by hovering over it in the search bar and clicking 'x', the time range dropdown is disabled. -Fixed - when I select a Quick Range filter or open the drop-down after a, the Time Range inputs are not updated and still say "now/d" - I felt 'Quick Range' and 'Time Range' should be mutually exclusive. If it is confusing I can fix it. - when I click the 'X' or clear button in the search bar, I would expect the time range selector to be reset to "All time" - Fixed - when I open the drop down and change the "FROM" time range, I would expect the "TO" input to still say "now" (it's automatically set to end of the day picked in "FROM") - This is intentional just to ensure that To date is not before From date - when I open the drop down and change either "FROM", "TO" or both, I would expect those inputs to reset if I closed the dropdown without hitting "Apply" (they are still populated when I open the dropdown again) - If it is not really annoying I would leave this to default behaviour. There are both advantages/disadvantages of clearing the fields and retaining the value ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user cestella commented on the issue: https://github.com/apache/metron/pull/796 I definitely like the testing section @merrimanr I'd like to see the test in existence in the PR split into the test cases that you suggest. Testing those time range quicklinks turn into sensible timestamp pairs shouldn't even really require an end-to-end test, just a quick set of unit-style tests of the javascript. ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/796
I tested this as well. First a couple things I'm curious about:
- when I open the date/time range picker why does it say "now/d"? I would
expect just "now" or "Now"
- when I first navigate to the Alerts UI or select "All time" why do I see
a timestamp clause in the query? Why not just leave it out?
I also found some issues. The first is related to time-range selections
that include 'Now' as part of the range (Last 7 days, Last 5 minutes, Today so
far, etc). This should be a sliding window so I would expect the search query
to be different every time the results are refreshed. For example when I
select "Last 5 minutes" this is the query that is immediately sent (only query
field is shown for simplicity):
```
{
"query": "timestamp:(>=1508426193000 AND <=1508426493000)"
}
```
After several cycles the query does not change although "Last 5 minutes" is
a different time window now:
```
{
"query": "timestamp:(>=1508426193000 AND <=1508426493000)"
}
```
I would also expect the time range to change in the time range selector
dropdown ("2017-10-19 10:16:33 to Now" for example) as well.
I also found the time range selector value isn't populated correctly when
loading a saved or recent search. For example when I select a recent or saved
search the time range clause is included in the search bar and the searches now
return a 500 error:
```
"query": "timestamp:\\(\\>\\=0 AND :\\ \\<\\=410244480\\) AND
source\\:type:bro"
```
Based on how it works normally I would expect there to be no time range
clause in the search bar and the time range dropdown to be populated correctly
instead.
A couple other minor issues:
- if I select a cell in the table to apply a filter, then remove the filter
by hovering over it in the search bar and clicking 'x', the time range dropdown
is disabled.
- when I select a Quick Range filter or open the drop down after a , the
Time Range inputs are not updated and still say "now/d"
- when I click the 'X' or clear button in the search bar, I would expect
the time range selector to be reset to "All time"
- when I open the drop down and change the "FROM" time range, I would
expect the "TO" input to still say "now" (it's automatically set to end of the
day picked in "FROM")
- when I open the drop down and change either "FROM", "TO" or both, I would
expect those inputs to reset if I closed the dropdown without hitting "Apply"
(they are still populated when I open the dropdown again)
As far as testing is concerned, this is my understanding of the tests
included in this PR:
- verify all the links and time/date inputs are present in the time range
selector
- verify the drop down label changes to the correct value when a Quick
Range is selected (time range below the label is not tested)
- verify the result count in the table changes when a Time Range is entered
This is a good start but I think we need more to cover the issues I found.
I would suggest adding tests to:
- ensure saved/recent searches includes and functions properly when a Time
Range/Quick Range is selected
- test all the Quick Range filters to ensure they produce the correct time
range (I realize this could be tedious to test using search results so I would
compromise and just test that the correct range is populated in the drop down
and time range inputs)
- add validations for the time range selector when filters are
added/removed in the search bar
- expand tests on setting "FROM" and "TO" inputs to cover the issues noted
above
---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/796 I tested the PR. The only issue I see is that when I paste the timestamp or manually type it into the boxes it overwrites it with the calendar entries. So essentially the only way to get the timestamp in is to enter it through the calendar selection. Other than that it works great. I tested the positive case and the negative case. The search functionality works. I am a conditional +1 on this, because I do want to be able to enter the timestamps manually (not through the calendar picker). If you can fix that I think this PR is good to go ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user james-sirota commented on the issue: https://github.com/apache/metron/pull/796 @ottobackwards @iraghumitra i already filed a feature request on that: https://issues.apache.org/jira/browse/METRON-1248 ---
[GitHub] metron issue #796: METRON-1224: Add time range selection to search control
Github user iraghumitra commented on the issue: https://github.com/apache/metron/pull/796 @ottobackwards that's a good suggestion can you create a ticket with some more details about new quick filters and any design that you have in mind. I would be happy to pick it up as a follow-on ---
