[Logs / Metrics UI] [NP followup] Discuss / amend shared Observability hooks

[Kerry350]

As part of #54583 the shared tracker hooks which previously lived within the infra codebase were moved to the observability plugin. The behaviour was changed to account for plugins (in this case usageCollection) now being accessed via the <KibanaContextProvider />.

During review @sqren pointed out that DX could be improved.

On this ticket we should gather opinions from Observability as a whole with regards to the API we want for these shared hooks. And then implement the outcome (this doesn't necessarily have to be by the metrics / logs team).

[elasticmachine]

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

[sorenlouv]

Thanks for taking this up. Maybe we should start outlining which shared hooks we have. My concerns was only about the useUiTracker hook.

[Kerry350]

Thanks for taking this up. Maybe we should start outlining which shared hooks we have. My concerns was only about the useUiTracker hook.

👍 The only shared hooks we have at the moment are the tracking hooks: useUiTracker, useTrackMetric, and useTrackPageview.

I think we can quite easily roll these concerns into one hook (especially with useTrackPageview just being a wrapper around useTrackMetric).

So I guess the big question is the API we want.

Also, are we happy with the assumption that all Observability plugins will setup their own <KibanaContextProvider /> with the correct service dependencies to make use of these hooks. OR, do we want the shared hooks to not involve themselves with calling useKibana and instead receive the plugin(s) as a parameter?

[sorenlouv]

What about a new context provider like?

<UITrackerProvider app="apm">
  // rest of APM app
</UITrackerProvider>

or perhaps better naming:

<TelemetryProvider app="apm">
  // rest of APM app
</TelemetryProvider>

Then downstream we can call useTrackPageview or useTrackMetric without specifying the app name.

[Kerry350]

👍 I like this:

<TelemetryProvider app="apm">
  // rest of APM app
</TelemetryProvider>

Regarding the one plugin dependency we have on usageCollection, should this be provided by the <TelemetryProvider /> too to make things more explicit, or the<KibanaContextProvider /> still?

<TelemetryProvider app="apm" plugins={{ usageCollection }}>
  // rest of app
</TelemetryProvider>

The hooks would then access everything from the Telemetry context.

vs

<TelemetryProvider app="apm">
  <KibanaContextProvider services={{ usageCollection }} >
    // rest of app
  </KibanaContextProvider>
</TelemetryProvider>

The hooks would access the two contexts respectively.

I'm not strongly drawn to either really, every plugin is likely to be making use of <KibanaContextProvider /> anyway.

[sorenlouv]

I'm a little hesitant to couple TelemetryProvider and KibanaContextProvider together. Mostly because I can't remember what we use KibanaContextProvider for. If KibanaContextProvider is basically KibanaWideTelemetryProvider (with and oddly generic name) then YES! OTOH if KibanaContextProvider is a generic context provider for all things Kibana, then it's probably not so good to bundle them.

[Kerry350]

The <KibanaContextProvider /> is provided by the wider Kibana team as a way to access core and plugin services. There's also some extra sugar for things like uiSettings. Services can be accessed via the useKibana hook they provide (which is currently what these tracker hooks are using).

On the logs / metrics apps we've made use of these for all of our core / plugin access. However, there's nothing forcing apps to do so, so maybe you're right that requiring use of this as well is a negative.

[sorenlouv]

On the logs / metrics apps we've made use of these for all of our core / plugin access. However, there's nothing forcing apps to do so, so maybe you're right that requiring use of this as well is a negative.

I think it makes sense to consume KibanaContextProvider directly. But I'd be surprised if apps consumed it through a telemetry provider. That's all I'm saying :)

So I'm definitely in favor of this approach:

<TelemetryProvider app="apm">
  <KibanaContextProvider services={services} >
    // rest of app
  </KibanaContextProvider>
</TelemetryProvider>

[weltenwort]

Maybe I'm missing something but wouldn't binding the provider to the app name so high up in the hierarchy make tracking telemetry of embedded UI parts very difficult? I'm imagining APM embedding a logs stream, which contains its own fine-grained telemetry, which might then be incorrectly namespaced. Or is that a desired behavior?

If not, how about creating a factory for distinct provider/hook pairs to avoid such a collision?

// somewhere in apm/public

export const {
  ApmTelemetryProvider,
  useTrackApmMetric,
  useTrackApmPageview,
} = createTelemetryProvider('apm');

Would that be an improvement? 🤔

[sorenlouv]

Maybe I'm missing something but wouldn't binding the provider to the app name so high up in the hierarchy make tracking telemetry of embedded UI parts very difficult? I'm imagining APM embedding a logs stream, which contains its own fine-grained telemetry, which might then be incorrectly namespaced. Or is that a desired behavior?

That's a good point.

My biggest gripe with the current approach is that we went from the simple, stateless form:

trackEvent({ app: 'apm', name: 'save_agent_configuration' })

To the stateful form which requires two function calls:

const trackApmEvent = useUiTracker({ app: 'apm' });
// later in code
trackApmEvent({ name: 'save_agent_configuration' })

To avoid duplicating useUiTracker in certain places it is being passed around as a prop to components - I don't think this improves readability.

If not, how about creating a factory for distinct provider/hook pairs to avoid such a collision?

So instead of:

import { useUiTracker } from '../../../../../../../../../plugins/observability/public';
const trackApmEvent = useUiTracker({ app: 'apm' });
trackApmEvent({ metric: 'save_agent_configuration' });

it would be:

import { useTrackApmMetric } from '../../plugins/apm/public/telemetry';
useTrackApmMetric({ metric: 'save_agent_configuration' });

I like that 👍

[jasonrhodes]

So if I understand correctly, I think the goal of this shared telemetry code was something like: "Have a shared way for all of us to easily track events relying on the Kibana-provided telemetry tools, assuming all of our apps are loaded inside of the KibanaContextProvider and therefore shared components can rely on core-dependent hooks."

The only downside of the completely stateless approach is that it relies on every caller to remember the exact app name or else telemetry results will fork/split in unexpected ways.

I don't think we want to move these helpers back into our own plugins if the goal is to have a simple shared telemetry tool that we can each reach for. Honestly I kind of like what we have now, where you can create a partially-applied event tracker for your entire app. I'm also okay with having a "TelemetryAppProvider" that does essentially the same thing.

In both cases, I think shared components that want to track their own telemetry events would ship with their own event tracking in place, and would either override the "app" value in the track event call or would wrap themselves in their own TelemetryAppProvider before they export their component.

[jasonrhodes]

How about for now, to keep this simple and moving forward, we just use constants provided by the observability plugin, something like this:

import { TELEMETRY_APPS } from 'plugins/observability/public';

useTrackEvent({ app: TELEMETRY_APPS.APM, name: 'save_agent_configuration' });

I would also be perfectly fine with the observability plugin exporting the basic app trackers, but it'd be nice to keep them centrally located, I think:

import { useTrackApmEvent } from 'plugins/observability/public';

useTrackApmEvent({ name: 'save_agent_configuration' })

[sorenlouv]

@jasonrhodes I think both approaches work. The first one is how it used to be, and the second is like @weltenwort suggested but centrally located.
So that works for me 👍

[afgomez]

tl;dr If we want to have shared components (and I think we want), the best approach is this one.

---

@weltenwort had this question earlier:

I'm imagining APM embedding a logs stream, which contains its own fine-grained telemetry, which might then be incorrectly namespaced.

That is a good point. If I recall correctly we want to have shared components across apps, and IMO those components should track the app where they happen.

In the example that @weltenwort has proposed, I think it should be tracked under the "APM" app, because that's where the component is being rendered. Users might change behaviour on the same component across different apps. We want to know that.

The best approach for that case is this one.

<UITrackerProvider app="apm">
  // rest of APM app
</UITrackerProvider>

Later the useTrackMetric et al hooks can read the app from the provider, and send the metric under the right app. Compare this approaches.

// 1. Implicit app name through a provider. Does the right thing no matter the app.
function SomeComponent() {
  return <EuiButton onClick={() => useTrackMetric({ metric: 'click: something' })}>Button</EuiButton>;
}

// 2. `useAppNameTrackMetric`
// All events will be routed to the APM app, even if this component is used in other app
function SomeComponent() {
  return <EuiButton onClick={() => useApmTrackMetric({ metric: 'click: something' })}>Button</EuiButton>;
}

// 3. Explicit app name
// Same issue as previous. All events are routed to the APM app, even if this component is used somewhere else.
function SomeComponent() {
  return <EuiButton onClick={() => useTrackMetric({ app: 'apm', metric: 'click: something' })}>Button</EuiButton>;
}

One possible solution for this could be to pass either the app or the tracker as a prop, but I think that makes the DX worse. Now we need to remember to pass something to some components, but not always, and if we want to move a component to a shared space we need to refactor all metric usages.

// 2.
function SomeComponent({ tracker }) {
  return <EuiButton onClick={() => tracker({ metric: 'click: something' })}>Button</EuiButton>;
}

// Somewhere in APM
function SomeOtherComponent() {
  return (
    <>
      {/* This button is "normal" */}
      <EuiButton onClick={() => useApmTrackMetric({ metric: 'click: first button' })}>First button</Button>
      {/* This one is not */}
      <SomeComponent tracker={useApmTrackMetric} />
    </>
  );
}

[sorenlouv]

One possible solution for this could be to pass either the app or the tracker as a prop, but I think that makes the DX worse.

Completely agree 👍

[jasonrhodes]

@afgomez I see what you're saying, but I think some components will just want to track their own events ("how many times is this shared app used at all?" app: "observability" in that case, perhaps...) and in the cases where apps want to add their own tracking, they should just do it using the "on" handlers provided as usual.

// in MySharedComponent
export function MySharedComponent({ onClick }) {
  const localOnClick = (e) => {
    useTrackEvent({ app: "observability", name: "my_shared_component_clicked" });
    onClick(e);
  };
  return <EuiButton onClick={localOnClick}>My Shared Button</EuiButton>;
}

// in Logs where it gets used
<MySharedComponent onClick={() => useLogsTrackEvent({ name: "logs_shared_component_clicked" })} />

Anything more clever than that I think we should hold off on and see how other examples of telemetry event tracking emerge.

[jasonrhodes]

To wrap this up, I've created #60505 and I'm closing this as resolved for now. Thanks all!