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

Fix alert condition validations #3257

Merged
merged 14 commits into from Dec 28, 2016

Conversation

Projects
None yet
2 participants
@edmundoa
Member

edmundoa commented Dec 27, 2016

As described in #3247, the pluggable alert condition forms introduced in 2.2 are propagating data using the wrong data types to the server, which is also failing to fix or properly validate that data, resulting in wrong objects being written into MongoDB.

To avoid this situation, this PR is doing the following:

  • Convert values in the REST request according to the alert condition type definition
  • Check the alert condition configuration to ensure it follows the type definition
  • Standarize alert condition plugin definitions, so they look and work as other available plugins
  • Remove custom alert condition forms. It sounded like a good idea having custom forms and falling back to ConfigurationForm if no custom form was given, but in practice this does not work as well: the code for each form is slightly different, leading to validation issues, errors due to missing methods, or holding state in different components, etc. They may also look quite different, adding inconsistencies to the user as well. IMHO we need to sort out how we want them to look, and how the API should be, and then re-introduce them if needed
  • Improve alert condition descriptions, making more clear what each option do

Fixes #3247

@edmundoa edmundoa added this to the 2.2.0 milestone Dec 27, 2016

@edmundoa edmundoa requested a review from bernd Dec 27, 2016

@bernd bernd self-assigned this Dec 27, 2016

final Configuration configuration = new Configuration(parameters);
requestedConfiguration.check(configuration);
} catch (ConfigurationException e) {
LOG.error("Could not load alert condition <" + id + ">, invalid configuration detected.");

This comment has been minimized.

@bernd

bernd Dec 28, 2016

Member

I think we should include the alert condition title if possible in error messages to make it easier to find them.

This comment has been minimized.

@edmundoa

edmundoa Dec 28, 2016

Member

The only problem is that it may not be available (titles for alert conditions are only there since 2.1). Anyway, we can add it if available 👍

@@ -96,19 +93,21 @@ public Config() {
public ConfigurationRequest getRequestedConfiguration() {
final ConfigurationRequest configurationRequest = ConfigurationRequest.createWithFields(
new TextField("field", "Field", "", "Field name that should be checked", ConfigurationField.Optional.NOT_OPTIONAL),
new NumberField("time", "Time Range", 0, "Time span in seconds to check", ConfigurationField.Optional.NOT_OPTIONAL),
new NumberField("threshold", "Threshold", 0.0, "Value which triggers an alert if crossed", ConfigurationField.Optional.NOT_OPTIONAL),
new NumberField("time", "Time Range", 5, "Number of minutes to evaluate in the condition (0 evaluates condition in all messages)", ConfigurationField.Optional.NOT_OPTIONAL),

This comment has been minimized.

@bernd

bernd Dec 28, 2016

Member

I had a hard time to understand the description. Maybe something like "Evaluate the condition for all messages received in the given number of minutes"?

This comment has been minimized.

@edmundoa

edmundoa added some commits Dec 23, 2016

Move AbstractAlertCondition getNumber to Tools
That is a more appropiate place for this method.
Convert alert condition configuration values
Values coming from the API may not be using the right type, so we need
to ensure we convert them to what we expect.

Refs #3247
Verify alert condition configurations
Check if the configuration is valid against the requested configuration
for that alert condition, ensuring all required fields are set, and the
right data types are being used. If that is not the case, a
`ConfigurationException` will be raised.

Refs #3247
Try to get condition title from form
When using the fallback form component, the title is not stored in this
component's state, but in the form itself.
Only try to close the modal when possible
When using the fallback `ConfigurationForm` component, there is no close
method to call.
Remove pluggable alert condition forms
Having custom pluggable forms, and falling back to using
`ConfigurationForm` when none was provided sounded like a great idea,
but in the end it is tricky, as forms behaved in slightly different
ways, and they also asume the developer would follow certain implied
conventions.

As this way also feels inconsistent from a user's point of view, I
think it is best at this point to only use `ConfigurationForm` components
for now, and think of a better way of doing custom forms for alert
conditions in future versions.

@edmundoa edmundoa force-pushed the issue-3247 branch from f670ac0 to fba9183 Dec 28, 2016

@edmundoa

This comment has been minimized.

Member

edmundoa commented Dec 28, 2016

I took care of the mentioned issues and also rebased against master.

@bernd

bernd approved these changes Dec 28, 2016

LGTM 👍

@bernd bernd merged commit 9afd468 into master Dec 28, 2016

2 of 4 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
ci-web-linter Jenkins build graylog-pr-linter-check 1212 has succeeded
Details
licence/cla Contributor License Agreement is signed.
Details

@bernd bernd deleted the issue-3247 branch Dec 28, 2016

@bernd bernd removed the ready-for-review label Dec 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment