elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.69k stars 8.12k forks source link

Enhance Painless autocomplete suggestions with javadocs #93952

Open alisonelizabeth opened 3 years ago

alisonelizabeth commented 3 years ago

Problem

The Painless autocomplete feature implemented with the monaco editor only includes the method definition in the suggestion description. While this is helpful, it is incomplete, and there is room to improve the experience for users who are not Painless experts.

Example of current functionality:

Screen Shot 2021-03-03 at 11 40 21 AM

Proposal

We can enhance the description by incorporating javadocs. The updated description could look something like this:

Screen Shot 2021-02-16 at 2 20 39 PM

Affected apps: The editor is being consumed by the runtime fields editor, Painless Lab and Ingest Node Pipelines and any changes would automatically be applied to these apps.

Technical considerations

Javadocs are licensed under GPLv2 and therefore cannot be simply added to the existing generated autocomplete JSON.

In order to comply with this, we now need to bundle the autocomplete JSON (enhanced with javadocs) separately and consume it at runtime. There would also be an additional JSON file that has the ES javadoc contents only (Painless augmentation and helpers) that would need to be merged with the autocomplete JSON.

We currently have a script that copies the context definitions from the ES repo and reformats the data into a more consumable format for monaco. This logic would need now need to occur at runtime, likely as part of the getAutocompleteSuggestions() function. We would also need to update how the description field is generated, to incorporate the information included in the javadoc field (see JSON snippet below).

Proposed JSON format enhanced with javadocs ``` { "classes": [ { "name": "Appendable", "imported": true, "constructors": [], "static_methods": [], "methods": [ { "declaring": "java.lang.Appendable", "name": "append", "return": "Appendable", "javadoc": { "description": "Appends a subsequence of the specified character sequence to this Appendable. An invocation of this method of the form out.append(csq, start, end) when csq is not null, behaves in exactly the same way as the invocation out.append(csq.subSequence(start, end))", "parameters": { "csq": "The character sequence from which a subsequence will be appended. If csq is null, then characters will be appended as if csq contained the four characters \"null\".", "start": "The index of the first character in the subsequence", "end": "The index of the character following the last character in the subsequence" }, "return": "A reference to this Appendable", "throws": [ [ "IndexOutOfBoundsException", "If start or end are negative, start is greater than end, or end is greater than csq.length()" ], [ "IOException", "If an I/O error occurs" ] ] }, "parameters": [ "CharSequence", "int", "int" ], "parameter_names": [ "csq", "start", "end" ] } ] } ```
Additional ES javadoc contents ``` { "name": "CharSequence", "imported": true, "constructors": [], "static_methods": [], "methods": [ { "declaring": "org.elasticsearch.painless.api.Augmentation", "name": "replaceAll", "return": "String", "javadoc": { "description": "Replace all matches. Similar to Matcher#replaceAll(String) but allows you to customize the replacement based on the match." }, "parameters": [ "Pattern", "Function" ], "parameter_names": [ "pattern", "replacementBuilder" ] }, { "declaring": "org.elasticsearch.painless.api.Augmentation", "name": "replaceFirst", "return": "String", "javadoc": { "description": "Replace the first match. Similar to Matcher#replaceFirst(String) but allows you to customize the replacement based on the match." }, "parameters": [ "Pattern", "Function" ], "parameter_names": [ "pattern", "replacementBuilder" ] } ], "static_fields": [], "fields": [] }, ```
cjcenizal commented 3 years ago

Note that a key aspect of this is that we'll be asynchronously loading the javadocs JSON. By retrieving this JSON via an HTTP request and simply rendering it for the user, we're adhering to the GPL, which covers the javadocs.

jdconrad commented 3 years ago

@stu-elastic and I discussed this issue this morning. Given that we have decided to create an Elasticsearch REST API for contextual Painless suggestions, it may make more sense to serve Javadoc as part of this suggestion API. I think this is likely worth discussion in a meeting together given the potentially larger scope of the work. What do you think @cjcenizal @alisonelizabeth @stu-elastic ?

alisonelizabeth commented 3 years ago

I've scheduled a meeting to discuss this further.

rjernst commented 3 years ago

@alisonelizabeth @cjcenizal @jdconrad @stu-elastic Thanks for the discussion earlier. I wanted to clarify what I believe we agreed to in our discussion and offer some thoughts on the technical implementation from the Elasticsearch side.

What I believe we agreed to is:

I think it is important we distinguish the rendering of the suggestions in Kibana from the APIs called to gather the info used in that rendering. For this, I think there are performance reasons that the completions served up by Elasticsearch should be distinct from the javadocs of methods. There are two main reasons for this:

  1. The Javadocs don't change, so passing them back and forth is just extra data over the wire, over and over.
  2. The response time of a completion API is important in not giving the user a sense of lag. Javadocs are relatively huge, and will clearly slow that API down, or at least be a limiting factor in the speed we could serve completions.

So, while the rendering can have the the method/variable completions, along with javadocs popup for the particular method we are looking at, the Elasticsearch side should be two separate APIs. In our discussion it seemed Kibana preloading the javadocs available should be possible. @alisonelizabeth @cjcenizal can you confirm? Would it be possible to prefetch this based on the Elasticsearch version being communicated with, so that upon an upgrade a new version is fetched being calling autocomplete?

stu-elastic commented 3 years ago

Elasticsearch serving completions and javadocs.

Completion: https://github.com/elastic/elasticsearch/issues/49879 Javadocs: https://github.com/elastic/elasticsearch/issues/70783

alisonelizabeth commented 3 years ago

Thanks @rjernst for the great summary!

What I believe we agreed to is:

Kibana rendering full javadoc of methods in completion popup Elasticsearch serving completions and javadocs Look at changing completion rendering to include full method signature

👍 LGTM. I've gone ahead and scheduled a follow-up meeting to continue discussing the last point.

So, while the rendering can have the the method/variable completions, along with javadocs popup for the particular method we are looking at, the Elasticsearch side should be two separate APIs. In our discussion it seemed Kibana preloading the javadocs available should be possible. @alisonelizabeth @cjcenizal can you confirm?

Yes, I don't foresee a problem fetching the javadocs separately from the suggestions. Note that we (Kibana) will still have to map the javadocs to the provided suggestions on the fly.

Would it be possible to prefetch this based on the Elasticsearch version being communicated with, so that upon an upgrade a new version is fetched being calling autocomplete?

I think we're on the same page here, but I just want to clarify - my thought was that once the monaco editor component is mounted (i.e., rendered on the page), we would make a single request to ES to fetch the javadocs and store in memory. Then, as a user starts typing, we would begin making requests to the completion API, map the javadocs with the results, and return suggestions to the user. Let me know if you had a different idea here. We should always be communicating with the correct ES version.

rjernst commented 3 years ago

@alisonelizabeth That matches my thinking in general. My question on version was about the behavior when Elasticsearch is upgraded behind the scenes, ie during a rolling upgrade. I guess Kibana has no idea that the upgrade is happening, so can't take any additional action? In that case, it would seem we can't do more than what you described.

alisonelizabeth commented 3 years ago

Thanks for clarifying! I don't believe Kibana has awareness of rolling upgrades on the ES side, but I can reach out to the Kibana team to confirm. Kibana itself does not support rolling upgrades.