[App Search] Analytics - initial logic file & server API routes

[cee-chen]

Summary

This PR adds an initial AnalyticsLogic (missing a ton of values - currently just a few basic ones in here + the initial API call) and our API routes for making a call to ent-search's analytics/query endpoints.

This PR cannot be tested by itself, only code reviewed - it will be QA-able in shortly-upcoming PRs that add actual Analytics views & functionality.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[cee-chen]

@JasonStoltz - I implemented the reducer block suggestions you noted in #87561 (comment)! I like this a lot so far, feels much faster 🎉

[cee-chen]

There's a ton more reducers still to import, these are the primarily http/fetch-related ones that I know I'll be using soon.

I'm migrating reducers as-needed to try and catch ones we don't need and can potentially clean up from ent-search.

[JasonStoltz]

Good call. I like to migrate them as-needed as well.

[cee-chen]

loadAnalyticsData and loadQueryData are fairly similar in structure - I debated trying to DRY them out (e.g., a shared fn that gets passed an API URL/query/callback), but in the end decided it wasn't worth it. We likely aren't ever going to have more than just these 2 API calls so I prioritized straightforward code over more abstract

[JasonStoltz]

+1 I think that was a good choice

[cee-chen]

I dropped these from our types - logRetentionSettings and query weren't being used at all in the frontend, and I'm no longer using startDate, endDate, currentTag due to changes/refactors in our analytics header (different UI elements/components for the tags & datepicker).

[JasonStoltz]

Cool, that comment is helpful.

[cee-chen]

Some day in the future, it would be nice to audit our server side API code and figure out what all we actually are using / aren't using and clean our responses up. I also might mention GraphQL unironically once or twice to give Byron hives

[cee-chen]

fyi I changed this slightly from the ent-search URL to be more RESTful (analytics/query/xyz->analytics/queries/xyz). It also follows the URL pattern we use for our documents API (e.g.engine/documents/id, not /engine/document/id).

[JasonStoltz]

I'm good with all of this. None of my comments are blocking. Proceed as you see fit.

[byronhulcher]

I've found value in typing these sorts of default values for tests, it can catch stuff when the properties of these objects change

[cee-chen]

I'm not 100% convinced of this - our Kea logic should be type-checking for us and alerting if we have type issues. Not sure if ent-search has been updated to use makeLogicType, but in Kibana I definitely know it type checks my test files.

[byronhulcher]

Similar to above I'm not 100% you should be try/catch-ing here

[byronhulcher]

Do we expect loadAnalyticsData to throw an Error the http fails? It looks like loadAnalyticsData is setup to catch the any http errors. Alternatively, I think you should explicitly assert that loadAnalyticsData is throwing.

[cee-chen]

Do we expect loadAnalyticsData to throw an Error the http fails

Yes. Rejected promises as well as 400/500 responses from the server (in production) will trigger the catch block.

It looks like loadAnalyticsData is setup to catch the any http errors.

You can try this yourself if you pull the branch down, removing this try catch will cause the test to throw an error. I believe this actually might be because flashAPIErrors() re-throws the error if there isn't a valid message to display (we do this to catch Javascript syntax errors and other unexpected errors instead of silently swallowing them).

Alternatively, I think you should explicitly assert that loadAnalyticsData is throwing.

Unfortunately it's not quite that simple with Jest and async expects that are actually throws, the syntax is fairly bizarre and far more complex than this. I've struggled with this a lot in the past and am confident try/catch within the test is by far the easiest way to catch expected errors.

I definitely agree we could clarify the // Do nothing comment to be a little more helpful and indicate why we're doing nothing, although that would probably have to be a separate PR since I'm essentially copying a pattern we've done elsewhere in the codebase (originally set up by @JasonStoltz). We could also possibly extract this out to a helper which could help clear confusion in that regard.

[JasonStoltz]

It's complicated, IIRC.

You can't catch an exception thrown by a listener, like loadAnalyticsData.

I think it's cause you're not invoking the listener directly. They're on separate call stacks or something. It's like an evented system. Calling loadAnalyticsData broadcasts an event, and then 1 or more event listeners are triggered and execute. They're separate run contexts / call stacks / or however you would say that in JS. So you could wrap your code that triggers an event in a try / catch, and it will catch errors that occur while triggering that event, but it won't catch errors that occur in event handlers.

Does that much make sense?

So this code is actually misleading. You could actually move loadAnalyticsData outside of the try / catch and it would work ( I didn't test this, so please tell me to stuff it if I'm wrong 😰 )

        AnalyticsLogic.actions.loadAnalyticsData();
        
        try {
          await promise;
        } catch {
          // Do nothing
        }

So the point so far is that this test isn't actually testing that loadAnalyticsData throws correctly. This test is testing that the loadAnalyticsData correctly handles a rejected promise returned from http.get.

So here's the next challenge.

We need to tell http.get to return a reject promise so that we can test that scenario. And then we need to await that promise.

You can't do something like this:

(HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(Promise.reject());
await AnalyticsLogic.actions.loadAnalyticsData();

Because loadAnalyticsData doesn't return a promise. So we have to create the promise separately so we can hold a reference to it to await later:

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);
        await promise;

When you await a rejected promise, it will throw, so you gotta wrap it up:

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);

        try {
            await promise; 
        } catch {
          // Do nothing
        }

You may be tempted to run some assertions in the catch at this point..

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);

        try {
            await promise; 
        } catch {
          expect(flashAPIErrors).toHaveBeenCalledWith('error');
          expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();
        }

But I think you should generally avoid that ... because if for some reason your code ever stops throwing, then your code won't run the expectations, but the test will still pass, so you end up with a false positive.

        const promise = Promise.reject('error');
        (HttpLogic.values.http.get as jest.Mock).mockReturnValueOnce(promise);

        try {
            await promise; 
        } catch {
            // Do nothing
        }
        
          expect(flashAPIErrors).toHaveBeenCalledWith('error');
          expect(AnalyticsLogic.actions.onAnalyticsUnavailable).toHaveBeenCalled();

So that's generally how we arrived here. This could probably be improved.

Any ideas?

[JasonStoltz]

If you can get access to invoke a listener directly, that could simplify things.

[cee-chen]

Follow up PR: #88422

[byronhulcher]

Do we not have a routePaths or similar setup in Kibana?

[cee-chen]

We don't, there's some complexity there because it would need to live in the common/ folder to be shared between both our public and server folders, and so far I haven't been bothered to / seen a real need to DRY them out.

I'd definitely need to be convinced of the benefit there honestly, I find it more useful to have the explicit URL immediately available so I can check my devtools/XHR calls for debugging etc. and not have to hunt down whatever URL var the source is from (I'll admit this annoys me a decent amount in ent-search especially since our API routes are all programmatically generated so I have to go to a 404 page to even find our API routes).

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 4ab77fc

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
enterpriseSearch	1076	1077	+1

Async chunks

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

id	before	after	diff
enterpriseSearch	1.8MB	1.8MB	+3.9KB

History

• 💚 Build #98671 succeeded 6d08cb0

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