Improve highlighting by using highlight_query with all_fields enabled #9671

lukasolson · 2016-12-29T00:13:47Z

Fixes #2358.
Replaces #9091.

After some discussion, we decided that the way to have the most reliable and predictable highlighting experience would be to utilize the highlight_query option, and use the same query as the request body query, with the exception that all_fields is enabled for any query_string queries.

This also adds another advanced setting, doc_table:highlight:all_fields, which defaults to true. If set to false, then the highlighting won't use a separate highlight_query.

This should resolve all of the cases that we discussed in #9091.

Bargs

Highlighting seems to work really well now! I tried out all the scenarios from the previous ticket and everything was great. I left a few thoughts about implementation inline.

Could you also update the docs to include the new advanced setting in this PR?

Bargs · 2016-12-29T17:29:51Z

src/ui/public/courier/data_source/__tests__/highlight_body.js

+  const boolQuery = { bool: { must: [
+    { query_string: { query: 'foo' } },
+    { range: { '@timestamp': { gte: 0, lte: 0 } } }
+  ] } };


Could you format these so they're more readable?

Bargs · 2016-12-29T17:38:48Z

src/ui/public/courier/data_source/_highlight_body.js

+  if (_.has(clone, 'query_string')) {
+    clone.query_string.all_fields = true;
+  } else if (_.has(clone, 'bool.must')) {
+    clone.bool.must = clone.bool.must.map(getHighlightQuery);


I'm a little worried about this being too brittle. I know we talked about only worrying about the query_string queries kibana creates, but maybe that's not such a great idea. If we change the structure of the query discover is generating (or ES changes the API) we might break highlighting without knowing about it. I also noticed another bug related to this assumption:

The query entered into the query bar is valid, but the code here assumes bool.must is always an array.

What if we just recursively find any query_strings in the body and add all_fields to them, instead of making assumptions about the structure of the query body?

That's kinda tricky, because what if the user has a field named query_string and is using that field inside a query? Then we might be adding all_fields in a place where it's not even allowed.

If only we had some kind of intermediate representation to avoid such issues... :)

It's a good point, I'll try to think on it over the weekend.

I don't see a way to reliably do this without transforming the query into a normal form first either (which would be equivalent to an "intermediate representation"). ☹️

Bargs · 2016-12-29T17:46:55Z

src/ui/public/courier/data_source/_highlight_body.js

+    fields: {
+      '*': {}
+    },
+    fragment_size: FRAGMENT_SIZE


If doc_table:highlight:all_fields is false, should we set require_field_match to false here to maintain the old behavior? Or do you think having a default_field is the only reason why someone might turn all_fields highlighting off, in which case they probably do want to require field matches?

Whoa, I never saw this comment for some reason. You hit the nail on the head... I wouldn't expect someone to turn this off unless they had a default field set. What do you think?

I agree, I also can't think of a reason to turn it off unless someone wants to search a specific field by default and in that case they probably only want highlighting on that specific field. I was probably worrying too much about preserving the old behavior :)

Bargs · 2016-12-29T18:01:29Z

src/ui/public/courier/data_source/_abstract.js

@@ -389,6 +391,8 @@ export default function SourceAbstractFactory(Private, Promise, PromiseEmitter)
            recurse(agg.aggs || agg.aggregations);
          });
        }(flatState.body.aggs || flatState.body.aggregations));
+
+        highlightBody(flatState.body);


I like that moving highlighting to _flatten ensures that it works across discover and dashboard. But it also means that even Vis requests ask for highlighting. I wonder if we should try to avoid this? Since the highlighting logic is nicely modularized now maybe we could just reuse it in both discover and dashboard?

I also just realized this would overwrite any highlighting defined via the SearchSource.

I would definitely vote for not adding more magic to the data source. This piece of code has cost me so much time when writing the log context view before giving up on it. The more we add the more we restrict its reuse.

tbragin · 2017-01-04T05:04:03Z

Awesome progress! Functionally, highlighting works as I expect now, based on examples here #9091 (comment)

The only weirdness I noticed was a refresh issue with whether the expanded document view for one of the documents in the preview was open during a search or not. If it was, there doesn't seem to be a refresh in terms of highlighting.

lukasolson · 2017-01-04T16:21:41Z

@tbragin Does that happen in master as well, or is it limited to this PR?

tbragin · 2017-01-04T20:28:06Z

@lukasolson This issue seems to exists in master. Should I open a separate issue for it?

tbragin · 2017-01-04T21:04:51Z

Went ahead and filed a separate issue #9729

lukasolson · 2017-01-11T19:44:03Z

Okay, this is ready for review again. A couple of notes:

I'm not completely satisfied with the current implementation in that highlightRequest makes a call to _flatten. This means that any time we want to use this, _flatten will be called multiple times (once for highlighting and once to flatten for the actual request that gets sent). I'm open to suggestions here.
I changed highlightTags from an Angular constant to a simple object export, and changed highlightFilter from an Angular filter to just a plain function (getHighlightHtml). It wasn't being used anywhere in HTML anyway. I also added an additional test because it seems this was a condition that wasn't being tested.
I updated the getHighlightQuery logic to fix the bug that @Bargs mentioned above (when must is not an array).

Could you take another look, @Bargs and @weltenwort?

Bargs

I'm not completely satisfied with the current implementation in that highlightRequest makes a call to _flatten. This means that any time we want to use this, _flatten will be called multiple times (once for highlighting and once to flatten for the actual request that gets sent). I'm open to suggestions here.

I tend to agree with you here. It's also a bit fragile since a change to the query without a subsequent call to searchSource.highlightRequest will leave the highlighting in a stale state. What if instead we had a flag called highlightAll or something, which a consumer of searchSource could toggle on, indicating that they want to opt in to all fields highlighting? Then at _flatten time we check that flag and if it's set, generate the highlighting config, similar to how you had it before?

Also, have you looked into updating Dashboard to use this functionality in addition to Discover? At the moment highlighting doesn't work on terms entered into the Dashboard's query bar:

And one last thing: we'll still need to update the documentation with the new advanced setting option.

I haven't gone through the highlight module changes with a fine-tooth comb yet, but I wanted to leave these few high level comments before digging too deep.

Bargs · 2017-01-12T21:44:41Z

src/ui/public/highlight/highlight_html.js

+
+export default function getHighlightHtml(fieldValue, highlights) {
+  let highlightHtml = (typeof fieldValue === 'object')
+    ? angular.toJson(fieldValue)


Could this just be JSON.stringify? If we remove this one dependency on angular, the tests can be run standalone without Karma.

I can't remember exactly why this was put in here, but if I remember correctly, fieldValue could be an Angular object (with $$-prefixed values) and that's why we used angular.toJson.

Fair enough, not a big deal

lukasolson · 2017-01-27T17:29:53Z

@Bargs This is ready for another review. I didn't explicitly handle the dashboard case, but it looks to me like it's working there.

Bargs

Saw one minor doc typo, otherwise LGTM!

Bargs · 2017-01-27T21:04:40Z

docs/management/advanced-options.asciidoc

@@ -35,6 +35,8 @@ document.
 `discover:sampleSize`:: The number of rows to show in the Discover table.
 `doc_table:highlight`:: Highlight results in Discover and Saved Searches Dashboard. Highlighting makes request slow when
 working on big documents. Set this property to `false` to disable highlighting.
+`doc_table:highlight:all_fields`:: Improves highlighting by using a separate `highlight_query` that uses `all_fields` mode on
+`query_string` queries. Set to `false` if you are using a `default_field" in your index.


I think the double quote after default_field needs to be a backtick?

lukasolson · 2017-01-27T23:53:05Z

Should this only apply to 5.x, seeing as how in 6.x the default will be to disable _all?

Bargs · 2017-01-30T19:23:03Z

I think old indices will still have _all though right? I don't think _all will be completely removed until 7.0.

weltenwort · 2017-01-31T14:52:35Z

src/ui/public/courier/data_source/search_source.js

@@ -178,6 +181,11 @@ export default function SearchSourceFactory(Promise, Private, config) {
    });
  };

+  SearchSource.prototype.highlightRequest = function highlightRequest() {


I'm probably missing something here, but where is this ever called?

Oops, good point, that should have been removed.

weltenwort

Besides the left-overs it LGTM as far as the code goes. But I don't have full confidence in my understanding of the way this changes highlighting for the user. I would defer judgement to someone else in that regard.

weltenwort · 2017-02-03T13:56:50Z

src/ui/public/courier/data_source/search_source.js


 export default function SearchSourceFactory(Promise, Private, config) {
  const SourceAbstract = Private(AbstractDataSourceProvider);
  const SearchRequest = Private(SearchRequestProvider);
  const SegmentedRequest = Private(SegmentedRequestProvider);
  const searchStrategy = Private(SearchStrategyProvider);
  const normalizeSortRequest = Private(NormalizeSortRequestProvider);
+  const getHighlightRequest = getHighlightRequestProvider(config);


This might also be a left-over that can be removed.

weltenwort · 2017-02-03T13:57:01Z

src/ui/public/courier/data_source/search_source.js

@@ -58,13 +58,15 @@ import AbstractDataSourceProvider from './_abstract';
 import SearchRequestProvider from '../fetch/request/search';
 import SegmentedRequestProvider from '../fetch/request/segmented';
 import SearchStrategyProvider from '../fetch/strategy/search';
+import { getHighlightRequestProvider } from '../../highlight';


This might also be a left-over that can be removed.

Bargs · 2017-02-03T19:26:15Z

🎆 🎉 🎈 !

@Bargs

…elastic#9671) * Add all_fields to highlight query to improve highlighting * Refactor highlighting and move out of _flatten * Make changes as per @Bargs' requests * Add documentation about highlightAll setting * Fix docs typo * Remove unused function * Remove unused code

lukasolson · 2017-02-03T20:07:14Z

Backported to 5.x (5.3) in 18451e5.

Add all_fields to highlight query to improve highlighting

4effa5a

lukasolson added :Discovery review v6.0.0 labels Dec 29, 2016

lukasolson self-assigned this Dec 29, 2016

lukasolson requested review from weltenwort and Bargs December 29, 2016 00:13

Bargs assigned lukasolson and Bargs and unassigned lukasolson Dec 29, 2016

lukasolson mentioned this pull request Dec 29, 2016

Only highlight ALL matching fields if query doesn't target specific fields #9091

Closed

Bargs suggested changes Dec 29, 2016

View reviewed changes

tbragin added the Feature:Highlight Highlighting of content via query label Jan 4, 2017

Bargs added updates_needed and removed review labels Jan 5, 2017

Refactor highlighting and move out of _flatten

7191a07

lukasolson added review and removed updates_needed labels Jan 11, 2017

Merge branch 'master' into highlight-all-fields

74aa281

Bargs suggested changes Jan 12, 2017

View reviewed changes

lukasolson added the v5.3.0 label Jan 23, 2017

lukasolson added 3 commits January 26, 2017 13:02

Merge remote-tracking branch 'upstream/master' into highlight-all-fields

0a325af

Make changes as per @Bargs' requests

8081e24

Add documentation about highlightAll setting

4b49c8c

Merge branch 'master' into highlight-all-fields

765ab01

Bargs suggested changes Jan 27, 2017

View reviewed changes

Fix docs typo

a2accc5

Bargs approved these changes Jan 30, 2017

View reviewed changes

weltenwort reviewed Jan 31, 2017

View reviewed changes

Remove unused function

264ac9e

weltenwort approved these changes Feb 3, 2017

View reviewed changes

Remove unused code

436f486

lukasolson merged commit 909b8c7 into elastic:master Feb 3, 2017

Bargs mentioned this pull request May 12, 2017

Highlighting fails with a cryptic error when issuing a custom query #6287

Closed

This was referenced Jul 28, 2017

Use default_field in highlight_query instead of all_fields #13185

Closed

Remove highlight query #13231

Merged

lukasolson deleted the highlight-all-fields branch March 27, 2018 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve highlighting by using highlight_query with all_fields enabled #9671

Improve highlighting by using highlight_query with all_fields enabled #9671

lukasolson commented Dec 29, 2016 •

edited

Loading

Bargs left a comment

Bargs Dec 29, 2016

Bargs Dec 29, 2016

lukasolson Dec 29, 2016

Bargs Dec 29, 2016

weltenwort Jan 2, 2017

Bargs Dec 29, 2016

lukasolson Jan 11, 2017

Bargs Jan 11, 2017

Bargs Dec 29, 2016

Bargs Dec 29, 2016

weltenwort Jan 2, 2017

tbragin commented Jan 4, 2017

lukasolson commented Jan 4, 2017

tbragin commented Jan 4, 2017

tbragin commented Jan 4, 2017

lukasolson commented Jan 11, 2017

Bargs left a comment

Bargs Jan 12, 2017

lukasolson Jan 16, 2017

Bargs Jan 17, 2017

lukasolson commented Jan 27, 2017

Bargs left a comment

Bargs Jan 27, 2017

lukasolson commented Jan 27, 2017

Bargs commented Jan 30, 2017

weltenwort Jan 31, 2017

lukasolson Jan 31, 2017

weltenwort left a comment

weltenwort Feb 3, 2017

weltenwort Feb 3, 2017

Bargs commented Feb 3, 2017

lukasolson commented Feb 3, 2017 •

edited

Loading

Improve highlighting by using highlight_query with all_fields enabled #9671

Improve highlighting by using highlight_query with all_fields enabled #9671

Conversation

lukasolson commented Dec 29, 2016 • edited Loading

Bargs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbragin commented Jan 4, 2017

lukasolson commented Jan 4, 2017

tbragin commented Jan 4, 2017

tbragin commented Jan 4, 2017

lukasolson commented Jan 11, 2017

Bargs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson commented Jan 27, 2017

Bargs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson commented Jan 27, 2017

Bargs commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bargs commented Feb 3, 2017

lukasolson commented Feb 3, 2017 • edited Loading

lukasolson commented Dec 29, 2016 •

edited

Loading

lukasolson commented Feb 3, 2017 •

edited

Loading