[Logs Explorer][Discover] Move Logs detail into Discover codebase

[tonyghiani]

📓 Summary

With the https://github.com/elastic/observability-dev/issues/2888 project, we worked on a Discover extension point to add custom content as part of the document detail flyout, creating an ad-hoc overview tab focusing on log documents.

With the idea of having a single Discover app, we want to move this technical implementation into the Discover codebase and reduce the injection of custom content from external plugins.

✔️ Acceptance criteria

From the user point of view, no behaviour is expected to change, all the work here is purely an implementation detail and should consist of:

• Create a new DocView preset tab, moving the content for the overview tab into the unified_doc_viewer plugin and accordingly adjusting the implementation where necessary.
• Leave exposed a way to enable/disable this doc view only in the context of logs. Currently, a copy of the DocViewsRegistry instance used to register doc views is exposed through the extension point, to minimize the changes on the extension point we can add a flag to doc views to programmatically enable/disable a doc view based on context, leaving the door open to further configuration. This wouldn't necessarily be the final approach for this configuration.

[elasticmachine]

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

[weltenwort]

Makes sense to me, mostly. 👍 Thanks for spelling it out.

This works although creates a direct dependency on Discover from any consumer.

Currently I don't see a problem with that.

As this registry doesn't rely on particular lifecycle hooks, we could factor it out into a package which works as an IoC container for Discover features [...]

The registry would still need to be instantiated and provided by plugin in its lifecycle at some point. Otherwise we'd have to rely on global module state, which we certainly want to avoid.

Does that make sense?

[tonyghiani]

Agreed, I don't have the specs defined and working on it will probably spot some unknown unknowns, I'll draft a PR for this and we can discuss on it 👌

[flash1293]

Thanks for this issue, @tonyghiani - I agree with the plugin thing, but the general approach sounds good to me.

One thing I'm not sure about is how ai-assistant is reused by multiple consumers. I don't think we can rely on the fact that only one or the other context is active and the other is completely disabled in a single browser session. I guess it would be possible to somehow subscribe to context changes and unregister/register, but that seems brittle. As the flavor-layer in Discover will anyway know about security vs. observability, it looks like there should be oblt-ai-assistant and security-ai-assistant, and the flavor layer chooses which one to use instead of doing this outside of the discover code. Wdyt?

[tonyghiani]

@flash1293 sounds reasonable, as pointed out during the last sync it makes sense to make this registration context-aware and be able to somehow specify what flavours involve the usage of the registered feature. Not sure if I would mix the feature identifier with the flavour, as they represent different things (where to locate a feature in Discover vs which context should load the feature), but is all food for thought and I'll look into this while drafting a PR, thanks for the input!

[flash1293]

Thanks, don't take anything as given I'm writing here, just giving my first impression.

be able to somehow specify what flavours involve the usage of the registered feature

@tonyghiani Just to make sure - are you imagining something like this

discoverRegistry.registerFeature({
  id: 'ai-assistant', // Constrained available features, no generic `flyout` or `header` pointing to UI areas
  flavor: 'oblt',
  render: (discoverDeps) => <ObsAIAssistant />
})

or something like this:

discoverRegistry.registerFeature({
  id: 'ai-assistant', // Constrained available features, no generic `flyout` or `header` pointing to UI areas
  isActive: (discoverState) => ...true/false,
  render: (discoverDeps) => <ObsAIAssistant />
})

or something completely else?

Not sure if I would mix the feature identifier with the flavour, as they represent different things (where to locate a feature in Discover vs which context should load the feature)

Yeah, I guess in this world it would be different in the sense that the security and the observability assistant would be different features and the flavor layer makes the decision which one to use. If there are multiple places registering the same "feature", it's going into the direction of generic extension points again. As opposed to a technically necessary inversion of control that's very clear about the fact that it doesn't try to be a generic extension layer.

My understanding was that it's the observability-flavored flyout that is loading the observability ai assistant and it's the security-flavored flyout that's loading the security ai assistant, to keep this decision in the same spot (within the discover code in the flavor layer) instead of having it outside of it in the assistant plugin.

The bigger point here is probably - should anything outside of the Discover code ever know about flavors? I don't think so, but maybe I'm misunderstanding.

This exercise definitely helps me getting towards this shared mental model.

[tonyghiani]

Lots of good points @flash1293! To recap on your questions:

Just to make sure - are you imagining something like this

discoverRegistry.registerFeature({
  id: 'ai-assistant', // Constrained available features, no generic `flyout` or `header` pointing to UI areas
  flavor: 'oblt',
  render: (discoverDeps) => <ObsAIAssistant />
})

I imagine something closer to this compared to the second example, I would keep it to a minimum complexity until we need to extend it for a specific reason (e.g. providing an enabled property etc)

My understanding was that it's the observability-flavored flyout that is loading the observability ai assistant and it's the security-flavored flyout that's loading the security ai assistant, to keep this decision in the same spot (within the discover code in the flavor layer) instead of having it outside of it in the assistant plugin.

This is how I see it too, the decisions will be taken by Discover internally and load registered features according to the current flavour. This injection mechanisms for features should be ideally used only when there are issues with the dependencies and it's not possible to push features down into Discover.

The bigger point here is probably - should anything outside of the Discover code ever know about flavors? I don't think so, but maybe I'm misunderstanding.

Agreed, ideally the outside world should not care about this flavour system that much. My idea was to have the flavor (or whatever naming we'll use) as an optional parameter, where the default behaviour of registering a feature is that it applies globally to Discover, and in exceptional cases it can specify a scope (maybe a better name for this param?) for this feature in Discover.
Not sure how it will be in the long term the AI Assistant, but I believe it might convert into a more core Kibana feature, so we could have a single AIAssistant shared across Kibana that doesn't need differentiation based on the context. Just assuming on this, my main point is that we should always aim to avoid external registrations unless is critically required.

[weltenwort]

Good discussion - @flash1293's idea of specifying the kind of AI assistant in the id (security-ai-assistant and oblt-ai-assistant) has some merits:

• This would allow any flavor to render either or both as needed. The registration is simply a mechanism to provide access to the feature (or not if the providing plugin is disabled). They would potentially take different parameters too, so they're not equivalent substitutes for each other anyway.

The other approaches conflate some things unnecessarily:

• a flavor property on the registration: the registered features don't necessarily line up with the flavors. We might have logs and traces flavors, that both want to render Observability AI Assistant.
• an isActive flag/predicate on the registration: this would put the providing plugin in control of when it should be rendered, which is undesirable.

[tonyghiani]

Thanks for the additions @weltenwort 🙏 Regarding the pros/cons you mentioned:

a flavor property on the registration: the registered features don't necessarily line up with the flavors. We might have logs and traces flavors, that both want to render Observability AI Assistant.

Agree, and as mentioned above, when registering a feature the principal scenario is that Discover wants to embed it as a first-class citizen and be opinionated about its usage (I also agree on not using an isActive flag, Discover is in control of the conditions for rendering it).
The flavour/scope parameter could be optional to narrow down the scope of the feature to only some of the allowed flavours, in pseudo-code, I first imagined it as something like:

interface BaseDiscoverFeatureDescriptor {
  scope?: DiscoverFlavour[]; // When not provided, the feature is used as a core feature independently from the flavoured scope
}

interface AIAssistantFeature extends BaseDiscoverFeatureDescriptor {
  id: 'ai-assistant';
  render: (deps) => JSX.Element;
}

interface ServiceFeature extends BaseDiscoverFeatureDescriptor {
  id: 'service';
  getServices: (deps) => Services;
}

type DiscoverFeature = 
  | AIAssistantFeature 
  | ServiceFeature;

This would allow any flavour to render either or both as needed. The registration is simply a mechanism to provide access to the feature (or not if the providing plugin is disabled). They would potentially take different parameters too, so they're not equivalent substitutes for each other anyway.

It sounds reasonable and I don't exclude this option, in the end it feels like is having a statically scoped feature instead of considering it global, which is not in contrast with the approach I described earlier in this comment.

[weltenwort]

Thanks for the explanation. I'm curious what the advantage of an externally-controlled flavor restriction would be. And would this advantage outweigh the decrease in predictability (when reading the Discover code)?

[tonyghiani]

I understand the benefits of having more predictable behaviour on the Discover side.
You have a good point about keeping this granularly controlled in Discover, from a technical perspective it makes sense.

My initial concern was whether a feature should be scoped to some flavours based on business logic and decisions taken on the external app injecting the feature, which can then accordingly constrain/expand the scope of the feature, but it might be a problem we'd never have.

Is it desirable to have the Discover codebase aware of all these decisions at the moment of defining each flavour?

[flash1293]

Is it desirable to have the Discover codebase aware of all these decisions at the moment of defining each flavour?

That was my understanding - ideally everything would be in one place, but that doesn't work due to constraints of the plugin system, so we need this indirection, but it should explicitly not be used to make decisions about Discover behavior outside of Discover.

My initial concern was whether a feature should be scoped to some flavours based on business logic and decisions taken on the external app injecting the feature, which can then accordingly constrain/expand the scope of the feature, but it might be a problem we'd never have.

If there's a strong reason to do it at one point in the future we can reconsider, but I think/hope we can avoid that. Let's not make it a cross-plugin concept as long as we don't have to, this is definitely a trap we went into before.

[davismcphee]

Catching up on this now, thanks all for the input. I think the overall approach makes a lot of sense, and the principle of when we should use IoC (basically only out of necessity) makes sense too. Just two points of feedback from my end:

• I agree we'll likely need a plugin for this vs a package since the registry will presumably be stateful. I like the DiscoverFeatureRegistry name you proposed, and the idea of referring to these as "features" vs something like "extensions". It communicates that these are things Discover depends on and not a way to extend its behaviour.
• I originally thought incorporating a flavour into the definition could make sense, but I agree with the points made by @flash1293 and @weltenwort that separate identifiers would probably be best. It doesn't leak details of Discover's flavour system outside of Discover, and as mentioned similar features might take different parameters from Discover. The point @weltenwort made about multiple flavours consuming the same feature is a good one. Also in a scenario such as navigating through docs from a mixed dataset in the doc viewer, we might display different flavours of the doc viewer between docs, which may consume separate versions of similar features (hopefully not O11y AI assistant for one doc and Security AI assistant for another, but we'll see 😅).