Add query param to limit highlighting to specified length

[matriv]

Add a max_analyzed_offset query parameter to allow users
to limit the highlighting of text fields to a value less than or equal to the
index.highlight.max_analyzed_offset, thus avoiding an exception when
the length of the text field exceeds the limit. The highlighting still takes place,
but stops at the length defined by the new parameter.

Closes: #52155

[romseygeek]

Analyzer implementation LGTM, I'll defer to Jim and/or Mark on the API. Does it make sense to have a separate query parameter, rather than doing everything with the index setting? That is, would it be easier to use if we just changed things so that you always get the limiting filter added, and you don't get exceptions any more?

[romseygeek]

You can use try-with-resources here I think?

[matriv]

Thought about it but we need to tries since, something like that: https://gist.github.com/matriv/ca6ca5fe191c8e0b1e49af282ec90669

so I preferred the traditional way, but happy to change if you prefer the try-with-resources.

[romseygeek]

you'll need to do a version check here so that mixed-cluster highlighting works

[matriv]

Done it already, thx.

[elasticmachine]

Pinging @elastic/es-search (Team:Search)

[jimczi]

, I'll defer to Jim and/or Mark on the API. Does it make sense to have a separate query parameter, rather than doing everything with the index setting? That is, would it be easier to use if we just changed things so that you always get the limiting filter added, and you don't get exceptions any more?

+1 for a separate query parameter. I'd also like to remove that setting but not sure if we need to do it in the same PR. Maybe we can consider this new option independently of the fact that the setting to limit highlighting is not useful ?

[matriv]

, I'll defer to Jim and/or Mark on the API. Does it make sense to have a separate query parameter, rather than doing everything with the index setting? That is, would it be easier to use if we just changed things so that you always get the limiting filter added, and you don't get exceptions any more?

+1 for a separate query parameter. I'd also like to remove that setting but not sure if we need to do it in the same PR. Maybe we can consider this new option independently of the fact that the setting to limit highlighting is not useful ?

Sorry, I'm a bit confused @jimczi . We should add this query param as is already in this PR, correct?
I don't get the rest, could you please explain a bit more about the the setting to limit highlighting is not useful ?

[jimczi]

Can you describe what's implemented with an example ? What's the relation between the query parameter and the setting ? Do we need both ? My +1 was for a dedicated query parameter that is independent of the setting.

[markharwood]

would it be easier to use if we just changed things so that you always get the limiting filter added, and you don't get exceptions any more?

Taking the perspectives of the various parties involved and what they want:

1. The es cluster admin wants to shut down abusive apps with noisy failures (the point of the existing index setting).
2. The client app developer has a new setting to write a more responsible app which doesn't blow up.
3. The end user doing a search on big docs no longer gets errors if the app developer has used the flag but doesn't get a "truncated" warning.

The existing error message below for exceeding the index.highlight.max_analyzed_offset is not ideal.

 "The length of [text] field of [0] doc of [index] index has exceeded [xx] - maximum allowed to be analyzed for "
                + "highlighting. This maximum can be set by changing the [index.highlight.max_analyzed_offset] index level setting. "
                + "For large texts, indexing with offsets or term vectors is recommended!",

It should probably offer an alternative remedy. This is the opportunity for the admin to tell the client app developer to write a better app so we should advertise the new limit_to_max_analyzed_offset flag in this message rather than only suggesting upping the index.highlight.max_analyzed_offset limit.

With regards to point 3 - the client app developer might want to tell users when content is truncated. This could be done by testing if the length of the highlighted string is greater than the limit_to_max_analyzed_offset setting but this is not a reliable test. A truncated flag in the response would be more robust but maybe warning users that text has been truncated is not too important given the hit-or-miss nature of highlighters anyway.

[matriv]

@markharwood I had in mind to have a follow up PR to have this warning/flag regarding the "truncation" of the highlighting, since I agree this is useful to avoid confusion for the user. For "simple" users, I guess they will first try to highlight a large doc, get the error, and then use the flag to highlight up to the limit, so they are aware of what's going on, therefore, of course I need to add the new param to the error message as a suggestion.

As you mentioned in point 1. seem we want to have both setting on the index and the per-query param.

Maybe, if we have a query param that has a similar name with the setting like: max_analyzed_offset which overrides the index setting and uses the filter to stop highlighting and avoid exceptions? (@jimczi probably what you're suggesting?). To me this doesn't sound bad, but I think we need to consider that in this case, the query param doesn't only override the limit of the index setting, but also changes the behaviour (query now succeeds with truncated highlighting).

[matriv]

Reworded the exception message with the help of @romseygeek, and removed the mention to index offsets and term vectors.