[ML] AIOps: Log Rate Analysis V2 REST API, replaces references to term with item.

[walterra]

Summary

Update 2023-11-09

After the discussions we had I took another attempt at not duplicating all code but adapting the route handler in a way to support both API versions. The updated solution now uses conditional types to allow the handling of both versions. The more tricky bit turned out to be not the updated request body, but the response since it is an NDJSON stream where some messages were updated. In this case also the functions that create these messages were updated with conditional types to be able to create a message that fits the definition of the API version.

The API integration tests originally had these message identifiers in the expected section of their testData. I changed that to use helper functions that retrieve the expected messages from the stream according to the expected version. All API integration tests are run on both versions. The functional tests are run only on the newer version since the UI is expected to work with version 2 only.

To review, I'd recommend using the diff view that ignores whitespace changes: https://github.com/elastic/kibana/pull/170274/files?w=1

Original Description 2023-11-06:

This is a first attempt at versioning a more complex REST API endpoint because of changing request/response bodies. This is for the log_rate_analysis endpoint for the AIOps plugin. At this stage of serverless as well as this being an internal endpoint, to do the versioning isn't critical but we thought it's worth doing for practicing the process and having a reference later on on how to do it.

Working on this I discovered we probably have to deal with these trade offs:

• This route has a complex route handler. First I tried to avoid duplicating everything, but it quickly became tricky to support both versions within one handler because of the need to update the related TypeScript interfaces. Adapting the existing code to support both versions increases the risk of breaking the previous version. Because the old version will be deleted eventually once it's no longer used, that type of refactor might not be worth the effort.
• Duplicating the whole route handler made the versioning easier. There's some drawbacks though: For PR reviews, everything that's duplicated will show up as new lines, so for reviewers it's trickier to know what to look for. Because of the duplication into new files, this will break the history of the file and you'll have a harder time tracking changes via git blame.

On top of that, I suspect there's not one solution that can be applied to every type of version change we'll encounter with other routes. We might want to settle on a shared directory structure for example though. Some updates might be easier to do and might even be possible to do with a single handler that's able to adapt for the changed route, sometimes it might not be feasible.

For the versioning IMHO it's essential to have API integration tests in place that cover both old and new API version. Again, I went with duplicating the code in this case. More or less the same trade off described above applies to the test code and assertions.

Nice side effect of this PR: I cleaned up the stuff that's being exported from the common area from the plugin which helped reduce the client side asynce bundle of the aiops plugin by more than 200KBytes.

General feedback on the structure and approach very welcome of course.

Review note: The duplicated code doesn't add any new logic, it is just about renaming the relevent bits from using term references to item.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/ml-ui (:ml)

[alvarezmelissa87]

Looks like there is a lot of type casting needed with 'as' - is there a way to remove the need for that?

[walterra]

I looked into this and I think there is no way around it in this case, all the examples I found for conditional types do the same (or fall back to any). There is an issue about this in the TypeScript repro which describes this problem, the issue was closed as a design limitation: microsoft/TypeScript#22735

[alvarezmelissa87]

Gave this a test. Left a small comment but LGTM overall ⚡

[jgowdyelastic]

LGTM
I agree that this type based approach is an improvement over the last.
One observation, with the previous code duplication approach it would be easier to remove the old version (once enough time has passed).
With this approach, the version logic is more complicated and so removing previous versions will require more work.
However the git history problems caused by the code duplication approach outweigh any problems with this approach.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: bf82eef

Failed CI Steps

• Investigations - Security Solution Cypress Tests #7

Test Failures

• [job] [logs] Investigations - Security Solution Cypress Tests #7 / Alert details expandable flyout right panel should add to new case should add to new case

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
aiops	481	416	-65

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
aiops	589.2KB	378.2KB	-211.0KB

Unknown metric groups

API count

id	before	after	diff
@kbn/ml-agg-utils	99	95	-4

History

• 💔 Build #174586 failed ea79c8b
• 💔 Build #174529 failed 4759bd1
• 💔 Build #174491 failed 7194c95
• 💛 Build #173436 was flaky e2a7168
• 💚 Build #172373 succeeded a28b9c084a63b49a3e4f149398c585e9f70711db

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra