[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 👍