[Enterprise Search] Telemetry: refactor to Kea logic file

[cee-chen]

Summary

This refactors our sendTelemetry() utility function (that we primarily used for onClick actions) into a logic action, which allows us to not have to import/pass the http dependency in every sendTelemetry call.

Before:

const { http } = useValues(HttpLogic);

sendTelemetry({
  http,
  product: 'app_search',
  action: 'clicked',
  metric: 'button',
});

After:

const { sendAppSearchTelemetry } = useActions(TelemetryLogic);

sendAppSearchTelemetry({
  action: 'clicked',
  metric: 'button',
});

Hopefully the code speaks for itself and feels clearer / closer to the <SendTelemetry /> components. It also makes unit tests a little less verbose, as we have a lot more helpers in place for mocking Kea values/actions.

Regression QA

All of the following telemetry actions/events should correctly send 200 XHR requests to api/enterprise_search/stats:

• App Search
  • Header > clicking "Launch App Search" button
  • Empty engine state > clicking the "Create engine" button
  • Engine table > Clicking either the engine name link or "Manage" link
• Enterprise Search
  • Clicking a product card's "Launch" button
• Workplace Search
  • Header > Clicking "Launch Workplace Search" button
  • Overview > Clicking the "Add sources" button
  • Overview > Clicking the "Name your organization" link
  • Overview > Clicking a recent activity link

Checklist

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

[JasonStoltz]

Just a check, could this test pass with a false positive in the case where this action never threw?

Like if someone updated the telemetry method to use post instead of put, I think this would still pass... because put is never called, the action never throws, and this catch is never executed. In that case, this test should fail but it would pass.

Just thinking that something written like the following might be more resilient:

    it('throws an error if the telemetry endpoint fails', async () => {
      const promise = Promise.reject()
      mockHttpValues.http.put.get.mockReturnValue(promise);
      let message = "";
      TelemetryLogic.actions.sendTelemetry({ action: '', metric: '', product: '' });

      try {
        await promise;
      } catch (e) {
        message = e.message;
      }
      expect(message).toEqual('Unable to send telemetry');
    });

Actually, I'm wondering if this is already a false positive. As in, since this is not an async test method, is that catch ever even being triggered?

[cee-chen]

Ohh super good point about false positives/this not being an async fn! 👍 I like your proposed solution a lot, will definitely implement it!

[cee-chen]

You were super on point about it being a false positive! I added an expect.assertions(1) to the top of the test and confirmed that it had 0 assertions, which means the catch wasn't firing. Credit to this article for the nifty technique:

https://medium.com/@liran.tal/demystifying-jest-async-testing-patterns-b730d4cca4ec#a5c3

[JasonStoltz]

Why do you add these here if you just end up importing telementry_logic.mock directly in all of the tests?

[cee-chen]

Because that way you import a single __mocks__/kea.mock.ts to mock all possible shared logic file values/actions at once instead of having to manually and tediously mock every logic file that you need.

So for example, in product_card.test.tsx:

https://github.com/elastic/kibana/blob/1db25d72bdfbc9782ee76675a5256593160acdc4/x-pack/plugins/enterprise_search/public/applications/enterprise_search/components/product_card/product_card.test.tsx#L7-L8

This file calls both KibanaLogic and TelemetryLogic. Because we have this shared kea.mock.ts helper, we only have to import it once and now the test file gracefully handles all instances of useValues and useActions, returning sane defaults that we can either spy on or override as needed (via our exported mock*Values/mock*Actions objs).

[JasonStoltz]

This looks great.

You're right, it seems to greatly simplify things. I had one hesitation about one of your unit tests, but other than that it looks great.

Thanks for doing this.

[JasonStoltz]

Also, I did not QA this. Let me know if that is something you'll need.

[JasonStoltz]

I mentioned this in a comment in Slack, but repeating it here. I have this weird sense of guilt about this comment, like we're blaming Kea for doing something weird 😭

Actions are like events in Kea, and reducers and listeners are like event handlers.

I would not expect that triggering an event would return a value from an event handler. Therefore, I would not expect that dispatching an action would return a value from a listener.

I mean think about it, there could be multiple event listeners listening for that event, how would you even know which value to return from the action invocation if there are 5 different handlers or listeners running.

The same is true with Kea and Redux.

Kea abstracts some things away from Redux... like when you call an action in Kea, you are invoking it in a way that it appears like a direct invocation of the handler logic, and less like triggering an event:

sendTelemetry()

In Redux, it looks more like the following, which makes it less tempting to expect a return value:

store.dispatch({ type: 'sendTelemetry' })

This is basically a long winded way of saying if you shift your thinking a bit, this seems less like interference.

[cee-chen]

Thanks for taking the time to write that all out Jason!!! Totally agreed and I found that super helpful / learned a few new things.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: b1fb8eb

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
enterpriseSearch	435	436	+1

async chunks size

id	before	after	diff
enterpriseSearch	638.5KB	639.4KB	+1013.0B

History

• 💚 Build #85371 succeeded 589d65e63c4925b9f60b2ddd1a514a1821663af7
• 💔 Build #85357 failed 41de8355532d5867ce5a08caa7c431df61380969
• 💚 Build #84362 succeeded 1db25d72bdfbc9782ee76675a5256593160acdc4

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