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

Alert condition titles #2282

Merged
merged 18 commits into from May 31, 2016
Merged

Alert condition titles #2282

merged 18 commits into from May 31, 2016

Conversation

@dennisoelkers
Copy link
Member

@dennisoelkers dennisoelkers commented May 25, 2016

This PR allows users to add titles to alert conditions and makes them available for alarm callbacks.

Before, two different ways to retrieve alert conditions were offered.
One (which was used in the StreamResource) was basically returning plain
mongo objects from the database, the other one (as offered by
StreamService) returned proper AlertCondition instances. Removing the
first one in favor of the second one to consolidate these and correct
usage.
@dennisoelkers dennisoelkers added this to the 2.1.0 milestone May 25, 2016
@@ -50,8 +50,6 @@ public static MatchingType valueOfOrDefault(String name) {

String getContentPack();

Collection<AlertCondition> getAlertConditions();

This comment has been minimized.

@joschi

joschi May 25, 2016
Contributor

This would break existing plugins that are working with Graylog 2.0.x and which are using this method. Any reason to remove it and not simply mark it as deprecated (to remove it in Graylog 3.x)?

This comment has been minimized.

@dennisoelkers

dennisoelkers May 25, 2016
Author Member

The method, as it was before, was plain broken. It worked only because of type erasure, as it was actually returning a Collection<BasicDBObject>. It was only used implicitly by Jackson when serializing a Stream instance as JSON, so it was also using a different representation that any other place that was serializing an AlertCondition instance and it was only identical by accident.

It was also a duplicate of StreamService#getAlertConditions which goes the hard path of instantiating an actual AlertCondition. Unfortunately we cannot just move that logic to the Stream implementation class (or pass the list of correct alert conditions to its constructor) as the AlertCondition instance itself references the Stream which holds it.

It's not the cleanroom technique to just remove it for a minor version, but it's actually balancing between api breakage and offering a method that is just plain broken.

This comment has been minimized.

@joschi

joschi May 30, 2016
Contributor

Works for me. At least it'll be obvious for plugin authors and users when stuff breaks.

But please add this to the UPGRADING.rst file.

This comment has been minimized.

@dennisoelkers

dennisoelkers May 30, 2016
Author Member

Check!

@edmundoa edmundoa self-assigned this May 26, 2016
@@ -397,7 +397,7 @@ private StreamResponse streamToResponse(Stream stream) {
stream.getFields().get(StreamImpl.FIELD_CREATED_AT).toString(),
stream.getDisabled(),
stream.getStreamRules(),
stream.getAlertConditions(),
alertConditions,

This comment has been minimized.

@edmundoa

edmundoa May 30, 2016
Member

Not a big thing, but this may affect some code consuming the API: Alert conditions returned in here seem to have the type written in upper case letters, whereas before they used lower case. Alert conditions returned by StreamAlertConditionResource also use lower case for the type: https://github.com/Graylog2/graylog2-server/pull/2282/files#diff-51de2a31470f13c3ee6f29bc5129b989R157

I think the safest way to avoid any troubles is to simply convert the types in here to lower case, even if clients may already do so.

This comment has been minimized.

@dennisoelkers

dennisoelkers May 31, 2016
Author Member

It is a big thing, actually. Thanks for spotting it!

This comment has been minimized.

@dennisoelkers

dennisoelkers May 31, 2016
Author Member

_formatTitle() {
const alertCondition = this.props.alertCondition;
const alertConditionType = new AlertConditionsFactory().get(alertCondition.type);
const title = alertCondition.title ? alertCondition.title : `Untitled ${alertConditionType.title} condition`;

This comment has been minimized.

@edmundoa

edmundoa May 30, 2016
Member

I would simply leave out the title if not present, I don't see the need of adding that "Untitled" prefix. I think specially at the beginning will look really weird, as none of the conditions will have title, and all will contain the untitled prefix.

This comment has been minimized.

@dennisoelkers

dennisoelkers May 31, 2016
Author Member

I think it makes sense to add it, because it hints users migrating to this version that they can now give titles to their alert conditions.

This comment has been minimized.

@edmundoa

edmundoa May 31, 2016
Member

Then I suggest using the same styling as newly added titles. I think it achieves the purpose you mentioned, and people can see the difference between the title and the type of condition.

@@ -1,5 +1,5 @@
import React from 'react';
import { Well } from 'react-bootstrap';
import { Input, Well } from 'react-bootstrap';

This comment has been minimized.

@edmundoa

edmundoa May 30, 2016
Member

Unused import

}

return undefined;
},
render() {
const alertCondition = this.props.alertCondition || {parameters: {}};

This comment has been minimized.

@edmundoa

edmundoa May 30, 2016
Member

The default object is missing some spaces :nitpick:

return {
alertCondition: {
parameters: {},
}

This comment has been minimized.

@edmundoa

edmundoa May 30, 2016
Member

Missing trailing comma.

return (
<Well className="alert-type-form alert-type-form-message-count form-inline well-sm">
Title: <input ref="title" type="text" className="form-control" autoComplete="off" defaultValue={alertCondition.title}/>
{' '}
<small>This title can also be included in alert notifications.</small>

This comment has been minimized.

@edmundoa

edmundoa May 30, 2016
Member

I find this text a little confusing inside this form, as we mostly describe there how the condition should apply. I think adding some colour would help with that, I'll commit that so you can take a look.

@@ -37,8 +37,9 @@ const AlertConditionForm = React.createClass({
return (
<Well className="alert-type-form alert-type-form-message-count form-inline well-sm">
Title: <input ref="title" type="text" className="form-control" autoComplete="off" defaultValue={alertCondition.title}/>
{' '}
<small>This title can also be included in alert notifications.</small>
<span style={{ color: '#939393', marginLeft: 10 }}>

This comment has been minimized.

@dennisoelkers

dennisoelkers May 31, 2016
Author Member

I am kinda opposed to adding even more inline styling. We should at least put it in some locally used stylesheet file.

This comment has been minimized.

@edmundoa

edmundoa May 31, 2016
Member

I only do that sometimes when the styling is only used in one place. Anyway, you are right, I'll put it into some file 👍

This comment has been minimized.

getDefaultProps() {
return {
alertCondition: {
parameters: {},

This comment has been minimized.

@edmundoa

edmundoa May 31, 2016
Member

I just realised that in 2.0, creating a new alert condition provides default values for parameters, and after these changes we don't do that any more. To fix that, we should remove the empty object from here, as each specific alert condition form provides some default props if the parameters are not given.

}

return undefined;
},
render() {
const alertCondition = this.props.alertCondition || { parameters: {} };

This comment has been minimized.

@edmundoa

edmundoa May 31, 2016
Member

At the moment this default alert condition will never be used, as the getDefaultProps method will already give us a default alert condition.

edmundoa and others added 3 commits May 31, 2016
Put it alongside other alert styles in graylog.less
@edmundoa
Copy link
Member

@edmundoa edmundoa commented May 31, 2016

LGTM 👍

@edmundoa edmundoa merged commit 81a0e10 into master May 31, 2016
4 checks passed
4 checks passed
ci-server-integration Jenkins build graylog2-server-integration-pr 957 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 444 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@edmundoa edmundoa deleted the alert-condition-titles branch May 31, 2016
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

3 participants
You can’t perform that action at this time.