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

Simple query time range limit #1806

Merged
merged 15 commits into from Feb 18, 2016

Conversation

Projects
None yet
3 participants
@joschi
Copy link
Contributor

commented Feb 12, 2016

In order to guard the system against accidental queries covering a large time range, the query_time_range_limit setting now provides a limit for those queries.

If a search query exceeds the time range given in ´query_time_range_limit`, the query is modified to fit within that limit by adjusting the starting time.

@joschi joschi added this to the 2.0.0 milestone Feb 12, 2016

@bernd bernd self-assigned this Feb 15, 2016

@bernd

This comment has been minimized.

Copy link
Member

commented Feb 15, 2016

I was wondering why we apply the limit at the "outer" layer in the REST resources instead of the Searches class? This way we might miss some places. (i.e. existing dashboards)

@bernd bernd force-pushed the query-range-limit branch from 821fcde to be20773 Feb 17, 2016

joschi and others added some commits Feb 12, 2016

Implement simple query time range limit
In order to guard the system against accidental queries covering a large
time range, the "query_time_range_limit" setting now provides a limit for
those queries.
If a search query exceeeds the time range given in "query_time_range_limit",
the query is modified to fit within that limit by adjusting the starting time.
Start UI components for the System/Configurations page
Only components for the search config for now.

@bernd bernd force-pushed the query-range-limit branch from 7ade042 to 58d46d9 Feb 18, 2016


private static final Period DEFAULT_QUERY_TIME_RANGE_LIMIT = Period.ZERO;
private static final Map<Period, String> DEFAULT_RELATIVE_TIMERANGE_OPTIONS = ImmutableMap.<Period, String>builder()
.put(ISO_PERIOD_FORMAT.parsePeriod("PT5M"), "Search in the last 5 minutes")

This comment has been minimized.

Copy link
@joschi

joschi Feb 18, 2016

Author Contributor

Wouldn't it be easier to use the static factory methods of Period, e. g. Period.minutes(5)? Or do they result in different objects?

<Button bsStyle="info" bsSize="xs" onClick={this._openModal}>Update</Button>
</IfPermitted>

<BootstrapModalForm ref="searchesConfigModal"

This comment has been minimized.

Copy link
@edmundoa

edmundoa Feb 18, 2016

Member

The component feels too big and I would actually move this form outside, just to make things more clear.

}
}

options[idx][field] = value;

This comment has been minimized.

Copy link
@edmundoa

edmundoa Feb 18, 2016

Member

Here we are mutating the component's state directly, and this is highly discourage in the documentation: https://facebook.github.io/react/docs/component-api.html#setstate. You can use ObjectUtils.clone to clone the state object before modifying it.

period = `P${period}`;
}

update[field] = period;

This comment has been minimized.

Copy link
@edmundoa

edmundoa Feb 18, 2016

Member

Here we are modifying the state directly as well.

newTimerangeOptions[option.period] = option.description;
});

config.relative_timerange_options = newTimerangeOptions;

This comment has been minimized.

Copy link
@edmundoa

edmundoa Feb 18, 2016

Member

Here we are modifying the state directly as well.

let timerangeOptions = null;
let timerangeOptionsSummary = null;
if (this.state.timerangeOptionsList) {
timerangeOptions = this.state.timerangeOptionsList.map((option, idx) => {

This comment has been minimized.

Copy link
@edmundoa

edmundoa Feb 18, 2016

Member

I like to put these large blocks creating some jsx into a function for clarity, also keeps the render method short. That's something of personal taste, of course :)


_errorHandler(message, title) {
return (error) => {
let errorMessage;

This comment has been minimized.

Copy link
@edmundoa

edmundoa Feb 18, 2016

Member

You shouldn't need to do this here, promises are returning a FetchError object that should already have the right message set. If, for some reason, that is not working properly, we should adjust it here so all promises get the right message: https://github.com/Graylog2/graylog2-server/blob/master/graylog2-web-interface/src/logic/rest/FetchProvider.js#L7-L11

@edmundoa

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

Other than the mentioned issues, we should also try to keep the UI more or less consistent with the existing one. But we can fix that later on, of course 👍

bernd added a commit that referenced this pull request Feb 18, 2016

Merge pull request #1806 from Graylog2/query-range-limit
Simple query time range limit

@bernd bernd merged commit 5265b44 into master Feb 18, 2016

1 of 4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 658 has failed
Details
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
ci-web-linter Jenkins build graylog-pr-linter-check 149 has succeeded
Details

@bernd bernd deleted the query-range-limit branch Feb 18, 2016

joschi added a commit that referenced this pull request Apr 5, 2016

Implement simple query time range limit
In order to guard the system against accidental queries covering a large
time range, the "query_time_range_limit" setting now provides a limit for
those queries.
If a search query exceeeds the time range given in "query_time_range_limit",
the query is modified to fit within that limit by adjusting the starting time.

(cherry picked from commit 40a530b)

Backported from #1806

joschi added a commit that referenced this pull request Apr 6, 2016

Implement simple query time range limit
In order to guard the system against accidental queries covering a large
time range, the "query_time_range_limit" setting now provides a limit for
those queries.
If a search query exceeeds the time range given in "query_time_range_limit",
the query is modified to fit within that limit by adjusting the starting time.

(cherry picked from commit 40a530b)

Backported from #1806
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.