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

Implement QuickValue improvements #4205

Merged
merged 12 commits into from Oct 5, 2017
Merged

Implement QuickValue improvements #4205

merged 12 commits into from Oct 5, 2017

Conversation

@bernd
Copy link
Member

@bernd bernd commented Oct 4, 2017

This PR implements the features outlined in #4082.

WIP

quick-values

edmundoa and others added 3 commits Sep 15, 2017
- Add a config form that can be toggled
- Make order, limit and table size configurable

There are still problems with the ordering in the data table, at least.
@bernd bernd added this to the 2.4.0 milestone Oct 4, 2017
@edmundoa edmundoa self-requested a review Oct 4, 2017
@edmundoa edmundoa self-assigned this Oct 4, 2017
Copy link
Member

@edmundoa edmundoa left a comment

I added some inline comments.

name="order"
label="Top values"
checked={this.state.order === 'desc'}
onChange={this._onOrderChange('desc')}

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

We don't need to use a custom change method for this input. In this case we can use value=("asc"|"desc") in the top and bottom Inputs to get the same effect. In that way the labels of the radio inputs also work as expected, i.e. change the values.

@@ -14,6 +14,11 @@ const FormUtils = {
return input.value;
}
},

// Emulates a minimal input event that can be used with FormUtils#getValueFromInput()
inputEvent(name, value) {

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

This looks quite nasty and as far as I see it is safe to remove. If we really need it, then we should use CustomEvent to get the same result but with an event object.

This comment has been minimized.

@bernd

bernd Oct 4, 2017
Author Member

I will remove it. 😄

render() {
return (
<Row>
<form className="form form-horizontal" onSubmit={this._onSave}>

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

The form and fieldset tags should be inside Col, as is intended by bootstrap. Otherwise we may ran in some CSS issues.

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

There's no cancel button on this form and no obvious way of getting back to the previous screen. I would add a Cancel button at the end, to make it more obvious.

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

I would also like to get rid of the form-horizontal here and make the column size smaller. In my opinion the single column form avoids making your eyes jump from the label to the field and in this case we don't have that many inputs anyway:

screen shot 2017-10-04 at 15 28 06

At the very least we should reduce the column size, because the current inputs don't require that much space, and I think long inputs look even worse than some white empty space.

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

In the example I did above, I also used a bit of padding (15px), so it looks aligned with the pie chart in the quick values visualization.

value={this.state.tableSize}
labelClassName="col-sm-3"
wrapperClassName="col-sm-9" />
<Input type="radio"

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

These two radio buttons need a label. I would use "Sort options", but feel free to go with something else you find more appropriate.

if (this.state.data.length === 0) {
if (this.state.showVizOptions) {
inner = (
<QuickValuesOptionsForm limit={this.state.options.limit}

This comment has been minimized.

@edmundoa

edmundoa Oct 4, 2017
Member

Adding this component as children of the div below, which sets overflow: auto, breaks the layout and makes this scroll horizontally.

This comment has been minimized.

@bernd

bernd Oct 4, 2017
Author Member

I will fix that.

bernd added 6 commits Oct 4, 2017
- Add "Cancel" button to QuickValues configuration
- Fix horizontal scrolling in QuickValues configuration form
- Remove unneeded custom event handler for radio buttons
- Add label for radio buttons
- Remove now unused FormsUtils#inputEvent()
This puts the "sortOrder", "dataTableTitle", "dataTableLimit" and "limit"
props into the existing "config" prop. This makes it way easier to
handle both usages of the QuickValuesVisualization, on dashboards and
on the search page.
Copy link
Member

@edmundoa edmundoa left a comment

Looks good! Just added a couple more comments, but it's almost ready to be 🚢 ed

limit: PropTypes.number.isRequired,
tableSize: PropTypes.number.isRequired,
order: PropTypes.string.isRequired,
field: PropTypes.string.isRequired,

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

This prop seems to never being set, at least when creating the element in FieldQuickValues.

This comment has been minimized.

@bernd

bernd Oct 5, 2017
Author Member

Fixed it.

required
onChange={this._onChange}
value={this.state.tableSize} />
<Input label="Sort options">

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

I'm getting Warning: Failed prop type: Invalid prop 'children' supplied to 'InputWrapper'. in here. Can we just use <FormGroup> and <ControlLabel> instead?

<FormGroup>
   <ControlLabel>Sort options</ControlLabel>
   ...
</FormGroup>
required
onChange={this.props.onChange}
value={this.props.config.data_table_limit} />
<Input label="Sort options">

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

Same as above.

return (error) => {
let errorMessage;
try {
errorMessage = error.additional.body.message;

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

Are we always going to set a user-friendly body message on responses to this request? I know the way we handle errors is far from good, but this pattern we repeat in many places (also duplicating code all over the place) is also not that great. Error messages returned from the server are often too technical and obscure, and other times too vague for being shown to a non-technical operator.

To put an example, and I know we discussed this before. Selecting a stacked field in ES 2.x returns: Loading quick values failed with message: Unable to perform terms query script_lang not supported [painless] which is not helping me at all to know what went wrong with that, just making me confused.

At least I think we should have this kind of error handling abstracted somewhere and not repeating it in every store.

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

Also now that I look at it, we even accept a callback there for some reason, I guess c&p 😄

This comment has been minimized.

@bernd

bernd Oct 5, 2017
Author Member

Are we always going to set a user-friendly body message on responses to this request?

Nope we don't. I agree that we need some kind of centralized error handling where we can map error codes from the backend to user friendly messages in the UI. (or somthing similar)

Until we have this, showing a message like in the code snippet below makes it really hard to debug the problem because our exception handling in REST resources is mostly automatic and does not log anything to the server log.

UserNotification.error(`Loading quick values failed with status: ${error}`,
          'Could not load quick values');

So I personally rather have a message that the user might not understand correctly but which helps me to debug the problem. 😃

I completely agree regarding the code duplication. If we want to use this kind of error handling until we have a better one, we should put this into a helper function that can be shared.

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

Those errors may help solving the problem in some cases, but they are overly complicated, sometimes even include some Java exceptions and may even leak some information. Today for instance I was playing today with the email delivery and some details about the email server and so on may be leak from the API really easily without knowing it. As I said before, in my opinion that is no error message for a non-technical operator, which may operate Graylog.

Anyway, if we really want to change that behaviour or abstract it, we already have a helper for it:
https://github.com/Graylog2/graylog2-server/blob/master/graylog2-web-interface/src/logic/rest/FetchProvider.js#L14

We could add a method there without changing the default behaviour to make easier to access the body error message if it's available.

This comment has been minimized.

@bernd

bernd Oct 5, 2017
Author Member

Okay, I removed the custom error handler. This needs a centralized solution. 😃

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

Hehe, ok. I'll add a task to my to do to at least discuss it or make it easier to access the response message. 👍

</Input>

<Button type="submit" bsStyle="success" bsSize="small">Update</Button>
{' '}

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

We should use <ButtonToolbar> for this. From react-bootstrap docs:

Because React doesn't output newlines between elements, buttons on the same line are displayed flush against each other. To preserve the spacing between multiple inline buttons, wrap your button group in .

},

_onCancel(event) {
event.preventDefault();

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

I think we don't need to prevent the click event here.

inner = <Spinner />;
if (this.state.showVizOptions) {
inner = (
<div style={{ marginTop: 10 }}>

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

Can we add a CSS file to handle all these inline styles, please? :)

<Row>
<Col md={6}>
<form className="form" onSubmit={this._onSave}>
<fieldset style={{ paddingLeft: 15 }}>

This comment has been minimized.

@edmundoa

edmundoa Oct 5, 2017
Member

Can we add this into a CSS file, please?

bernd added 2 commits Oct 5, 2017
- Move stylesheet code into .css files
- Remove unneeded "e.preventDefault()"
- Avoid Input component nesting and use FromGroup and ControlLabel
- Use ButtonToolbar instead of manually adding spaces
Copy link
Member

@edmundoa edmundoa left a comment

LGTM 👍

@edmundoa edmundoa merged commit 1bbd7f4 into master Oct 5, 2017
2 of 5 checks passed
2 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 573 is running
Details
ci-web-linter Jenkins build graylog-pr-linter-check 1985 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@edmundoa edmundoa deleted the issue-4082-1 branch Oct 5, 2017
@edmundoa edmundoa mentioned this pull request Oct 5, 2017
4 of 5 tasks complete
edmundoa added a commit that referenced this pull request Oct 5, 2017
* Fix linting errors

* Migrate FieldQuickValuesStore to js/reflux

* First stab at making QuickValues configurable

- Add a config form that can be toggled
- Make order, limit and table size configurable

There are still problems with the ordering in the data table, at least.

* Fix sorting when bottom N is configured

* Add stacking support to the /terms endpoints

* Incorporate PR feedback

- Add "Cancel" button to QuickValues configuration
- Fix horizontal scrolling in QuickValues configuration form
- Remove unneeded custom event handler for radio buttons
- Add label for radio buttons
- Remove now unused FormsUtils#inputEvent()

* Add UI for stacked fields in quick values

* Remove debug console.log

* Refactor config handling in QuickValuesVisualization

This puts the "sortOrder", "dataTableTitle", "dataTableLimit" and "limit"
props into the existing "config" prop. This makes it way easier to
handle both usages of the QuickValuesVisualization, on dashboards and
on the search page.

* Adjust dashboard handling for new config options in QuickValues

* Implement review feedback

- Move stylesheet code into .css files
- Remove unneeded "e.preventDefault()"
- Avoid Input component nesting and use FromGroup and ControlLabel
- Use ButtonToolbar instead of manually adding spaces

* Remove custom error handler

(cherry picked from commit 1bbd7f4)
bernd added a commit that referenced this pull request Oct 12, 2017
Adding this in #4205 was a mistake. The fields need to be in a separate
file.
kroepke added a commit that referenced this pull request Oct 16, 2017
* Extract histogram boundary parsing into tools

* First stab at terms histogram API endpoints

* Add /terms-histogram endpoint for absolute and keyword searches

* Make interval optional and build a sane default based on the timerange

This uses the same logic we use in the web interface to construct an
interval.

* Initial QuickValues over time visualization

* Make sure the chart container is using the complete width

* Add link to go back from histogram to the overview

* Disable animations

* Rename StackedChartVisualization to QuickValuesHistogramVisualization

This is nowhere near a generic component at the moment.

* Move some code from SearchResource into a helper so we can reuse it

* Move some code from QuickvaluesWidgetStrategy into base class

We will reuse this soon.

* Add QUICKVALUES_HISTOGRAM widget strategy

* Make it possible to add the quickvalues histogram to dashboards

* Move quick values config out of QueryConfiguration

Adding this in #4205 was a mistake. The fields need to be in a separate
file.

* Only show data table limit if not qv histogram

* Remove unused "show_chart_legend"

* Restrict the chart legend to max 5 entries

Otherwise we would overwrite the x axis label.

* Improve QV histogram rendering based on GraphVisualization

* Handle resizing issues

* Fix chart size issue where the chart gets smaller every new render

We have been passing this.CHART_MARGINS into the chart and dc.js was
modifying the values. So on every new chart render the chart got
smaller.

Use an immutable value for this.CHART_MARGINS.

* Account for y label in margin

* Fix terms/termsHistogram bug with stacked fields

Wrap terms query with stacked fields in an additional filter to ensure
that the main and stacked fields actually exist in the message. Otherwise
we would get "null" values in the result.

* Cleanup QuickValuesHistogramVisualization

Removing unused variables and simplify some code.

* Cleanup QuickValues configuration forms

* Fix bucket sorting in quickvalue histogram
kroepke added a commit that referenced this pull request Oct 16, 2017
* Extract histogram boundary parsing into tools

* First stab at terms histogram API endpoints

* Add /terms-histogram endpoint for absolute and keyword searches

* Make interval optional and build a sane default based on the timerange

This uses the same logic we use in the web interface to construct an
interval.

* Initial QuickValues over time visualization

* Make sure the chart container is using the complete width

* Add link to go back from histogram to the overview

* Disable animations

* Rename StackedChartVisualization to QuickValuesHistogramVisualization

This is nowhere near a generic component at the moment.

* Move some code from SearchResource into a helper so we can reuse it

* Move some code from QuickvaluesWidgetStrategy into base class

We will reuse this soon.

* Add QUICKVALUES_HISTOGRAM widget strategy

* Make it possible to add the quickvalues histogram to dashboards

* Move quick values config out of QueryConfiguration

Adding this in #4205 was a mistake. The fields need to be in a separate
file.

* Only show data table limit if not qv histogram

* Remove unused "show_chart_legend"

* Restrict the chart legend to max 5 entries

Otherwise we would overwrite the x axis label.

* Improve QV histogram rendering based on GraphVisualization

* Handle resizing issues

* Fix chart size issue where the chart gets smaller every new render

We have been passing this.CHART_MARGINS into the chart and dc.js was
modifying the values. So on every new chart render the chart got
smaller.

Use an immutable value for this.CHART_MARGINS.

* Account for y label in margin

* Fix terms/termsHistogram bug with stacked fields

Wrap terms query with stacked fields in an additional filter to ensure
that the main and stacked fields actually exist in the message. Otherwise
we would get "null" values in the result.

* Cleanup QuickValuesHistogramVisualization

Removing unused variables and simplify some code.

* Cleanup QuickValues configuration forms

* Fix bucket sorting in quickvalue histogram

(cherry picked from commit d5fd35d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants