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

Provide pipeline-based message decorator #41

Merged
merged 5 commits into from Jul 14, 2016

Conversation

Projects
None yet
2 participants
@dennisoelkers
Member

dennisoelkers commented Jun 23, 2016

This is still WIP but already open for remarks.

@dennisoelkers dennisoelkers added this to the 1.0.0 milestone Jun 23, 2016

@dennisoelkers dennisoelkers force-pushed the provide-message-decorator branch from 10f2b56 to 63430c1 Jun 29, 2016

@kroepke

This comment has been minimized.

Member

kroepke commented Jul 1, 2016

We could implement a InterpreterListener to make Graylog2/graylog2-server#2408 (comment) easier

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Jul 1, 2016

Good idea. I am wondering how we pipe back the information to the consumer (the web interface) without leaking implementation details. I will check the details how the field list is constructed to find that out.

@kroepke

This comment has been minimized.

Member

kroepke commented Jul 1, 2016

AFAICS the field list comes directly out of the SearchResponse which needs a SearchResponseDecorator. I'm wondering if we can somehow unify those two decorators while still keeping it simply for plugin authors.

new NoopInterpreterListener());
final ResultMessage outMessage = ResultMessage.fromMessage(message, inMessage.getIndex(), inMessage.getHighlightRanges());
results.add(outMessage);

This comment has been minimized.

@kroepke

kroepke Jul 1, 2016

Member

Here we also need to check for Message#getFilteredOut before adding it to the new result set in case the rule dropped this message.

This comment has been minimized.

@kroepke

kroepke Jul 1, 2016

Member

Alternatively we could use an interpreter listener and merely hide the filtered messages in the UI.
This gets trickier by the minute :)

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 1, 2016

Member

So they are not actually removed from the list but flagged instead?

This comment has been minimized.

@kroepke

kroepke Jul 1, 2016

Member

Yes, that would be an option.
Right now all the statistics about the result set are off, of course, when messages are simply removed.
I guess this is a kind of a special case, the common one would be to somehow mutate messages to include new or changed field content, and not dropping entire messages.

However, for the use case to filter messages based on security rules for example, this could still be useful.
We probably need to come up with a better description though.

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Jul 1, 2016

AFAICS the field list comes directly out of the SearchResponse which needs a SearchResponseDecorator. I'm wondering if we can somehow unify those two decorators while still keeping it simply for plugin authors.

You mean something like this? :)

I removed that again, because it seemed a bit overkill to me, but I think it makes sense in a few more ways (like providing information to the UI which fields were changed/added/deleted/what their former content was etc., so it is transparent/switchable in the ui), but I kept this out of the first implementation to prevent it getting too complex and evolve it instead.

@dennisoelkers dennisoelkers force-pushed the provide-message-decorator branch from 63430c1 to 5203214 Jul 5, 2016

@kroepke

This comment has been minimized.

Member

kroepke commented Jul 5, 2016

Yeah, I think we need to unify the decorators to make the UI more usable.
If we are adding fields to messages we can't display them in the table right now. The detail view also doesn't know that the field is synthetic so it allows searching for the field value, which will fail.

For the alpha I could live with leaving it like it is implemented now, but for a final I would vote for changing it.

@kroepke kroepke self-assigned this Jul 11, 2016

@dennisoelkers dennisoelkers force-pushed the provide-message-decorator branch from 5203214 to 7213b08 Jul 14, 2016

return new MessageCollection(fullyProcessed);
}
public List<Message> processForPipelines(Message message, String msgId, Set<String> pipelines, InterpreterListener interpreterListener) {

This comment has been minimized.

@kroepke

kroepke Jul 14, 2016

Member

I'd like to refactor this in a future change, because the semantics of this refactored call are a little surprising:

  • the message parameter is mutated and needs to be reused by the caller
  • the return value are only newly created messages and do not contain the original message

While this makes sense in the original call site for other users it's quite surprising. Technically everything's correct, though.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 14, 2016

Member

Hm, yeah. I am a bit surprised now that I did this way too. I am checking why and if it's possible to change for this PR already, because I don't like it at all.

This comment has been minimized.

@kroepke

kroepke Jul 14, 2016

Member

I think this is simply because of the structure of the original implementation.
As we need to revisit the interpreter anyway (and we know it's not ideal in performance) I'd leave it as it is for now.

@kroepke

This comment has been minimized.

Member

kroepke commented Jul 14, 2016

lgtm, the other changes we'll do incrementally in more PRs 👍

@kroepke kroepke merged commit da4f1ec into master Jul 14, 2016

@kroepke kroepke deleted the provide-message-decorator branch Jul 14, 2016

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