HWKMETRICS-566 Power up the external alerter #727
Conversation
jshaughn
force-pushed
the
hwkmetrics-566
branch
from
860b387
to
d497fba
Compare
|
retest this please |
jshaughn
force-pushed
the
hwkmetrics-566
branch
3 times, most recently
from
df2f188
to
2df52db
Compare
|
@stefannegrea @lucasponce This is now ready, I think, for a code review prior to merge. Since the meeting I've cleaned things up a bit and incorporated feedback:
Please take a look, time permitting. |
| } | ||
| this.expression.eval(); | ||
| } catch (Exception e) { | ||
| throw new ExpressionException("Invalid eval [" + eval + "]: " + e.getMessage()); |
There was a problem hiding this comment.
Perhaps a IllegalArgumentException instead to propagate this one com.udojava.evalex.Expression.ExpressionException ?
There was a problem hiding this comment.
+1, replaced in two places so as not to propagate the 3rd party Exception class.
| import com.udojava.evalex.Expression; | ||
| import com.udojava.evalex.Expression.ExpressionException; | ||
|
|
||
| public class ConditionEvaluator { |
There was a problem hiding this comment.
If ConditionEvaluator is dealing with the expression, perhaps a BNF representation of valid expressions here is good.
It has not to be complete but some lines would definitely help.
For example, I put a similar one in this other alerter:
https://github.com/hawkular/hawkular-alerts/blob/master/hawkular-alerts-engine-extensions/hawkular-alerts-events-aggregation/src/main/java/org/hawkular/alerts/extensions/Expression.java#L31
There was a problem hiding this comment.
Good point, a couple of the new classes were missing class-level jdoc. The supported Expression syntax is more fully defined at the EvalEx page, I have created class jdoc that points to it, as well as adds more info. I also added class jdoc for ConditionExpression.
| case uptimeRatio: | ||
| return data.getUptimeRatio().doubleValue(); | ||
| case samples: | ||
| return data.getSamples() * 1.0; |
There was a problem hiding this comment.
I normally used the cast operator for this. I guess this is for converting to double, isn't it ?
There was a problem hiding this comment.
Multiplying by a double literal always returns a double, so a cast is not necessary.
|
|
||
| // First, collect the queries | ||
| List<String> queries = new ArrayList<String>(); | ||
| Matcher m = Pattern.compile("q\\(.*?\\)").matcher(eval); |
There was a problem hiding this comment.
Minor optimization.
If this one pattern is going to be used often you could make it static as part of the class.
There was a problem hiding this comment.
+1, create a static Pattern.
|
|
||
| public QueryFunc(String query) { | ||
| super(); | ||
| Matcher m = Pattern.compile("q\\((.*?),\\s*(.*?)\\)").matcher(query); |
There was a problem hiding this comment.
Same here. Pattern can be static to the class.
There was a problem hiding this comment.
+1, created a static Pattern.
| * the evaluation is performed once (per run of the ConditionExpression). EACH means that data for each | ||
| * metric will be aggregated separately and the evaluation is performed once per metric. Note that if multiple | ||
| * queries are involved in the eval expression that only metrics common to each query will be evaluated. | ||
| * The default is ALL. |
There was a problem hiding this comment.
I have read the comment, but I have some fear that perhaps I could misinterpreted the behavior, perhaps a couple of lines with examples of ALL/EACH to clarify could help.
There was a problem hiding this comment.
I have added a more verbose comment to hopefully make things more clear. This same information will be in the documentation.
| private static final Logger log = Logger.getLogger(ConditionExpression.class); | ||
|
|
||
| /** 10mn */ | ||
| public static final String DEFAULT_FREQUENCY = "10mn"; |
There was a problem hiding this comment.
"mn" means "minutes" here ? Why don't use just "m" ? In other DSL, the some-sort-standard prefixes are d/h/m/s etc.
There was a problem hiding this comment.
"mn" is the suffix used by Metrics' Duration. I question their decision as well but I am only conforming to their documented Duration type: http://www.hawkular.org/docs/rest/rest-metrics.html#_custom_string_formats.
|
|
||
| private static final Integer THREAD_POOL_SIZE = 20; | ||
|
|
||
| ScheduledThreadPoolExecutor expressionExecutor; |
There was a problem hiding this comment.
Note to self.
ThreadPools are growing.
I guess that at some point it could be nice to have a wrapper for these classes and put more descriptive names to them.
Then when we dump a JVM we can trace better the threads involved.
Just a side comment, no action required, but it is not first time that I think on it.
There was a problem hiding this comment.
+1, they are everywhere.
|
|
||
| if (evaluator.evaluate()) { | ||
| try { | ||
| Data externalData = Data.forString(externalCondition.getTenantId(), |
There was a problem hiding this comment.
So, I am not 100% sure if I understand well some details of the logic.
But in EACH mode, the ExternalCondition will expect and Event and in ALL mode a Data to trigger the Alert.
Am I right ?
There was a problem hiding this comment.
No, the reason you are confused is that this is a bug, leftover from the conversion from Data to Event sending. Thanks for pointing it out. Now the code sends Events consistently.
| // Average over the last 1 minute > 50, check every minute | ||
| Query qNow = Query.builder("qNow").metric(metricId).duration("1mn").build(); |
There was a problem hiding this comment.
I would also add tests over the REST API through plain JSON too.
Perhaps the same but with plain text.
There are not support for extensions with Java clients, and also, I am not 100% if python/ruby clients are tested against ExternalCondition (probably not).
So, these JSON tests will be the closest example to a live example.
It would help to point people into the feature.
There was a problem hiding this comment.
I'm not exactly sure what that test would exercise. This code is already using REST to post the ExternalCondition. Are you concerned that python/ruby clients may not be able to post an ExternalCondition using a JSON string for the expression value?
There was a problem hiding this comment.
I know that groovy is unbeatable for these tests where Java objects are "JSON-lized" under the hood in a pretty straightforward way.
But experience showed us when someone else needs to use this feature, and we point them to these examples, they are not very useful, as they need to access via Java API.
I think these tests are great and we should continue using but what I do is to introduce some additional test where instead of an object we define the json representation in raw format.
That helps to catch up with the technology.
And point potential users to take the test as a valid example.
There was a problem hiding this comment.
OK, you are talking about this from the perspective of a code example. OK, I can create one like that. In the formal documentation I have a JSON example as well.
|
@lucasponce Muchas gracias for the review! I have pushed a commit with the changes (all but the requested JSON ITest example). |
|
With the review from @lucasponce and the follow-up commits I think I can remove the DoNotMerge label. @stefannegrea, or anyone, it is still open for more review comments or questions if you have them. |
|
Re-added DNM, I want to use only the the new tag query string, so waiting on PR 725... |
jshaughn
force-pushed
the
hwkmetrics-566
branch
from
b6db7a6
to
04471f7
Compare
|
@stefannegrea @lucasponce, I have removed the DNM label. I have rebased the PR on master, applied final changes wrt the new tag query work, and sqashed. Assuming travis is happy and if there are no further comments, I think this can be merged along with the associated documentation PR (hawkular/hawkular.github.io#265). |
- Introduces a new mechanism for defining conditions using flexible expressions with embedded stats queries. - Replaces the previous expression syntax. Note: - Introduces use of a new 3rd party component, EvalEx. A nifty tool for handling expression evaluation. - https://github.com/uklimaschewski/EvalEx - Added 'samples' to AvailabilityBucketPoint - good for consistency, and useful for determining upCount - Add Percentile.toString() for better debugging - Supports the new metrics tag query expressions
jshaughn
force-pushed
the
hwkmetrics-566
branch
from
04471f7
to
aa717d1
Compare
|
Thank you @jshaughn! |
flexible expressions with embedded stats queries.
Note:
tool for handling expression evaluation.