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

Decorate search results #2408

Merged
merged 17 commits into from Jul 8, 2016

Conversation

Projects
None yet
3 participants
@dennisoelkers
Member

dennisoelkers commented Jun 23, 2016

First version of message decorators. Not completed yet, but already open for comments/remarks.

You need Graylog2/graylog-plugin-pipeline-processor@63430c1 for it to work for now, as this is the only message decorator that is provided at the moment.

The general idea of the implementation is to provide a MessageDecorator interface, which is utilized in the SearchResource to process ResultMessage lists before they are encapsulated in a SearchResult and returned to the user for a search request. In general, a MessageDecorator can do anything when processing a message list, even adding or removing whole messages.

Those decorators are added to a search result view from the sidebar. You can add one or more decorators to a search result. For now these are persisted globally (so every user sees it), but separated by stream (or no stream at all).

REST-wise, decorators can be turned on/off using the boolean decorate-query parameter, defaulting to true.

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

@bernd

This comment has been minimized.

Member

bernd commented Jul 1, 2016

It would be cool if you could elaborate on the implementation a bit. That would make it a bit easier to review. 😃 Thanks!

@kroepke

This comment has been minimized.

Member

kroepke commented Jul 1, 2016

I noticed that added fields from a decorator do not show up in the fields section in the sidebar.
They probably need some special handling because you cannot search for them because they might not exist (or contain the right data) in the search index.

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Jul 1, 2016

I elaborated on the implementation a bit in the description. :)

</Col>
<ConfigurationForm ref="editForm"
key="configuration-form-decorator"
configFields={this.props.typeDefinition.requested_configuration}

This comment has been minimized.

@kroepke

kroepke Jul 1, 2016

Member

I would prefer if plugins could (optionally maybe?) supply their custom configuration component instead of having to fit everything into the configuration forms.
From a UX perspective I think especially the pipeline decorators would benefit from this.

We can probably postpone this until a second iteration on the feature to avoid delaying releasing this forever.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 4, 2016

Member

👍 I had the same in mind, but postponed it. I also think that we should just render a configuration form per default if no configuration component is supplied.

This comment has been minimized.

@kroepke

kroepke Jul 5, 2016

Member

Makes sense!

@dennisoelkers dennisoelkers force-pushed the decorate-search-results branch from f4ffce5 to 508603d Jul 5, 2016

@Override
protected void configure() {
Multibinder<MessageDecorator> messageDecoratorMultibinder = Multibinder.newSetBinder(binder(), MessageDecorator.class);
//messageDecoratorMultibinder.addBinding().to(UpperCaseDecorator.class);

This comment has been minimized.

@kroepke

kroepke Jul 5, 2016

Member

I guess we can remove that line.

@ImplementedBy(DecoratorProcessorImpl.class)
public interface DecoratorProcessor {
List<ResultMessage> decorate(List<ResultMessage> messages, Optional<String> stream);
SearchResponse decorate(SearchResponse searchResponse, Optional<String> stream);

This comment has been minimized.

@kroepke

kroepke Jul 5, 2016

Member

I'm not sure whether we really want both of these methods.
Currently, if a decorator creates a new field or additional messages, the metadata in SearchResponse is incorrect because of how it is assembled.
Additionally it seems like decorate(SearchResponse) includes what decorate(List<ResultMessage>) does. Both have access to the list of messages.

I'd vote for simplifying the interface to only process SearchResponse. I assume the first method is there to make it easier to implement a decorator, but I think then it becomes much harder to automatically keep the search metadata in sync (because we need to track the field names, result counts etc).

Additionally I think the result needs to be somehow annotated to allow the UI to hide the "search for field content" button: for synthetic fields there won't be anything to search for.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 5, 2016

Member

That is exactly the rationale behind this, ease of implementing a decorator vs. completeness. I also share your conclusion of using only the latter, but in the next iteration.

@JsonProperty(FIELD_HUMAN_NAME)
public abstract String humanName();
@JsonProperty(FIELD_IS_EXCLUSIVE)
public abstract boolean isExclusive();

This comment has been minimized.

@kroepke

kroepke Jul 5, 2016

Member

This seems like it is unused/erroneously copied from inputs :)

import java.util.List;
import java.util.stream.Collectors;
public class UpperCaseDecorator implements MessageDecorator {

This comment has been minimized.

@kroepke

kroepke Jul 5, 2016

Member

Unused? Seems like a no-op, too.

public SearchResource(Searches searches, ClusterConfigService clusterConfigService) {
public SearchResource(Searches searches,
ClusterConfigService clusterConfigService,
DecoratorProcessor decoratorProcessor) {
@@ -33,6 +33,10 @@ const ConfigurationForm = React.createClass({
newState.values = $.extend(newState.values, values);
this.setState(newState);
},
/*shouldComponentUpdate(nextProps, nextState) {

This comment has been minimized.

@kroepke

kroepke Jul 5, 2016

Member

left over from experiments?

@@ -34,216 +34,223 @@
*These should all be in the form of "group:action", because {@link Permissions#allPermissionsMap()} below depends on it.
* Should this ever change, you need to adapt the code below, too.
*/
public static final String USERS_CREATE = "users:create";

This comment has been minimized.

@kroepke

kroepke Jul 5, 2016

Member

I would prefer not to make formatting changes in this PR.

@kroepke

This comment has been minimized.

Member

kroepke commented Jul 5, 2016

With the exception of the formatting changes in RestPermissions the patch looks good to me for a first alpha. The rest we can do with additional PRs to keep them a bit smaller and focused.

@bernd any comments from your side?

@bernd

This comment has been minimized.

Member

bernd commented Jul 5, 2016

@kroepke what about the following?

Apart from that and the formatting changes: LGTM 👍

@dennisoelkers dennisoelkers force-pushed the decorate-search-results branch from 3c20514 to 449ad54 Jul 6, 2016

@kroepke

This comment has been minimized.

Member

kroepke commented Jul 8, 2016

lgtm we'll do more work in follow up PRs

@kroepke kroepke merged commit 619461a into master Jul 8, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 1066 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 552 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

@kroepke kroepke deleted the decorate-search-results branch Jul 8, 2016

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