Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 spEnrichedFilter #60
Add spEnrichedFilter #60
Changes from 3 commits
ba71200
fc495fd
d9cd827
cd5e8a4
39c4c8c
d09af8a
1469b99
5a5002a
03793bf
c5f7064
0e019f7
e98d953
8428a31
3b10912
2d46288
79ebc62
09a8e0d
9e49dfd
83f36db
1c40689
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first read the PR words, I thought it was multiple statements split with
|
but now I realise it's a single field with multiple options. I think it might be worth adding multiple field filters rather than multiple values per field, as that offers more flexibility without too much additional complexity (a little more though).e.g.
Rather than
MESSAGE_TRANSFORMATION=spEnrichedFilter:{field}=={value1}|{value2}|...
we haveMESSAGE_TRANSFORMATION=spEnrichedFilter:{field}=={value1}|{field}=={value2}|{field2}=={value25}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So following our chat - some clarification. An individual filter can only deal with one field, but can match n values. So an individual filter can either:
OR
However, we can apply as many filters as we like in a row - essentially chaining the above one after another. So your above example would be configured like so:
So if we have conditions which depend on multiple fields, we can do it as long as we require both conditions to be met. If we require that one or the other condition is met, we currently cannot configure that. So, if the example requirement was:
Then we'd be out of luck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should definitely make sure the input is valid. It'd be pretty easy to make a mistake when configuring this. Even if it's just ensuring all the characters are valid and it's been parsed resonably, if not then responding with an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - needs testing but as we discussed moving this section of code out of the returned function and into the body of
NewSpEnrichedFilterFunction
should achieve what we need.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
For me this makes it clearer what this does rather than flipping a boolean. I think this is worth the extra check for the sake of readability, especially since will only be called once per filter match. If you don't want the
if
thenshouldKeepMessage = !isNegationFilter
is clearer than flipping the same boolean.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the brevity of
shouldKeepMessage = !isNegationFilter
, but I think you're right that theif
statement is much better for readability so going with that. :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite clear, marking it as neither a success or a failure. It seems a little odd that a "transformation" can Ack a message though, it feels like a bit of an unexpected side effect of a transformation.
I wonder if we can take the same logic of the transformation section and pull it into a filter section in the code base to keep the two concerns separated? This might also afford us some future flexibility in filtering without having to try and squeeze it into the same shape as a transformation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think I agree, but would consider it out of scope of what we're doing here - which is to implement whatever sensible filtering we can in the short term.
I think it makes sense to separate the concept of a filter from the concept of a transformation. The only time I can really see this becoming a problem is if a filter were to depend on a transformation... But current implementation doesn't allow for this anyway... So I think certainly food for thought and a more nuanced design is well worth thinking over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a stop gap how do you feel about extending the interface to return a filtered message list so that we can handle what to do with the filtered set outside of the transformations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth considering... I did knock the idea about in my head, but ultimately figured that if we're gonna just ack them and ignore them, it's more efficient to do it straight away rather than pass more data around.
Having said that I'm not opposed to doing it that way for this instrumentation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further reflection on doing this - I think I am actually opposed to doing it this way, for the simple reason that it involves a lot of changes in a lot of places that we'll presumably want to revert back should we redesign it out:
Ultimately, it's not that these aren't worth doing, but that's as much work or more than just implementing Paul's suggestion that filters are done separately from transformations - and if we're to decide that's the best design, we'd need to undo all of the above in order to re-implement against that design.
Thoughts @jbeemster ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just popping an update here for clarity since I'm working on another branch atm, and we've progressed the discussion elsewhere.
We have discussed this and leaving things as they are isn't really an option - so we'll need to refactor something. At the moment am leaning towards Josh's suggestion of modifying the transformationResult model to allow a 'filtered' slice of messages, which are subsequently acked outside of the function.
Separating filters out completely means that we can't share information across a filter and a transformation, but filters can be much more powerful if they're allowed to depend on transformations IMO.