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

Add $(sum), $(min) and $(max) template functions #1037

Merged
merged 5 commits into from
May 5, 2016

Conversation

MrAnno
Copy link
Collaborator

@MrAnno MrAnno commented May 2, 2016

This PR contains three new template functions, which can be used together with correlation plugins such as correlation-parser, grouping-by() parser and PatternDB.
These correlation plugins create message contexts that contain multiple messages.

The newly introduced $(sum), $(min) and $(max) functions receive one argument, and operate on message contexts. The argument of the template function will be used to calculate the summation, the minimum and the maximum value of the context.


Example usage with the grouping-by() parser:

parser p_groupingby {
  grouping_by(
    key("${KEY}")
    timeout(5)
    aggregate(
      value("MAX" "$(max ${NUMBER})")
      inherit-mode("none")
    )
    inject-mode("pass-through")
  );
};

This parser groups messages based on their ${KEY} value.
After the group is complete (timeout is reached), the aggregation will be triggered and ${MAX} will contain the maximum value of the ${NUMBER} fields in the message group.

}

void
assert_template_format_with_context_msgs(const gchar *template, const gchar *expected, LogMessage **msgs, gint num_messages)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit long line there

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@bazsi
Copy link
Collaborator

bazsi commented May 2, 2016

This is great and very useful stuff which I'd liked merged. My notes are no biggies, but would be great to see at least some of these to be addressed.

@MrAnno
Copy link
Collaborator Author

MrAnno commented May 2, 2016

I'm going to fix these problems tomorrow. Thank you for the review.

@MrAnno MrAnno force-pushed the ctx-template-funcs branch 2 times, most recently from f0521e7 to 1715890 Compare May 3, 2016 14:37
@MrAnno
Copy link
Collaborator Author

MrAnno commented May 4, 2016

I modified multiple parts based on @bazsi's and @bkil-syslogng's review notes. Can I squash these fixups?

@bazsi
Copy link
Collaborator

bazsi commented May 4, 2016

The followups look great, please squash them.

There's one unaddressed review note, which is not a showstopper, so can go in without addressing it, e.g. it's up to you.

The function that gets the first value from the list has the same loop as the aggregation function. Presumably this is because the aggregators are expecting two values. This can be eliminated by using an explicit initialization value for the accumulator.

For maximum aggregation it can by thr int64 minimum value, for sum it is zero and so on.

This is how it is solved in functional languages.

MrAnno added 5 commits May 5, 2016 09:50
Example usage:

parser p_groupingby {
  grouping_by(
    key("${KEY}")
    timeout(5)
    aggregate(
      value("SUM" "$(sum ${NUMBER})")
      inherit-mode("none")
    )
    inject-mode("pass-through")
  );
};

Signed-off-by: László Várady <laszlo.varady@balabit.com>
Example usage:

parser p_groupingby {
  grouping_by(
    key("${KEY}")
    timeout(5)
    aggregate(
      value("MAX" "$(max ${NUMBER})")
      inherit-mode("none")
    )
    inject-mode("pass-through")
  );
};

Signed-off-by: László Várady <laszlo.varady@balabit.com>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
Signed-off-by: bkil-syslogng <tamas.nagy@balabit.com>
Signed-off-by: László Várady <laszlo.varady@balabit.com>
@MrAnno MrAnno force-pushed the ctx-template-funcs branch from e1cae1e to cdd328d Compare May 5, 2016 07:59
@MrAnno
Copy link
Collaborator Author

MrAnno commented May 5, 2016

Originally, I created the _tf_num_get_first_valid_arg function to eliminate any "first_found" boolean flag in the loop of minimum and maximum. I like the idea of the explicit initialization to int64's min and max value, but there is one ugly corner case:

What should I do when all values are "NaN" in the context? I think it would be nasty to return int64-min or int64-max in that case.
Also, if I want to detect this corner case, I should use a flag again.

@bkil-syslogng
Copy link
Contributor

@bazsi I've offered the same alternative in person, and we've discussed it with @MrAnno that it has drawbacks. For example, you either don't support INT max/min as a value and treat it as completely extremal, or you must keep track of whether we have already found a valid value or not. If we don't encounter a value, we return an empty string (Nothing), otherwise we return the stringified number (Maybe Int). The latter would take extra compute cycles compared to the optimized solution of @MrAnno. The former isn't that elegant.

And what would you do for sum? An empty sum could be defined as 0, but what about a median or average? We both see what you want to do, but we don't see the benefit of your proposed improvement.

On the other hand, the iteration might be abstracted again on a functional basis if it hurts that much - I'd love to see more Haskell code in C anyway.

Note that we've implemented it just like foldl1 (hand compiled a composition with a filter/catMaybes). foldl would need the additional initialization as you have mentioned.

@bkil-syslogng
Copy link
Contributor

bkil-syslogng commented May 5, 2016

Okay, as per the above, I'll give my 👍. I have further improvements in the pipe, but let's not block this feature with those for now. I'll submit them separately. Here's a work in progress about what I had in mind:
https://github.com/bkil-syslogng/syslog-ng/tree/f/ctx-template-funcs2

So if I'm reading @bazsi correctly, he also gave a plus one:
"There's one unaddressed review note, which is not a showstopper, so can go in without addressing it, e.g. it's up to you."

@ihrwein @dnsjts want to merge it?

@ihrwein
Copy link
Contributor

ihrwein commented May 5, 2016

👍 from me, I'll push the merge button

@ihrwein ihrwein merged commit bad4d03 into syslog-ng:master May 5, 2016
@ihrwein
Copy link
Contributor

ihrwein commented May 5, 2016

@fekete-robert : Could you document this new feature, please? Thanks!

bazsi added a commit that referenced this pull request May 20, 2016
This merge adds $(average) template function & further stylistic improvements to #1037
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants