Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Observability] Create context container to enable Observability plugin registry function #68642

Merged
merged 12 commits into from
Jun 15, 2020

Conversation

cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes commented Jun 9, 2020

closes #68534

Creates a context registry on the Observability plugin. The idea is that any plugin can registry its own way to fetch data.

Registring:

plugins.observability.dashboard.register({
  appName: ObservabilityApp,
  fetchData: (params: FetchDataParams) => Promise<FetchDataResponse[]
  hasData: () => Promise<boolean>
});
  • fetchData: Returns the data that will be used to display the charts on the Overview page.
  • hasData: Returns a boolean indicating if there's any data available in the APP indices. It'll be used to define if the Landing page should be opened instead of the Overview page, in case of all APPs return false.
    • terminateAfter: 1 can be used in the ES request since no data needs to be returned.
    • When a user doesn't have read privilege to the indices, it should return false.

@cauemarcondes cauemarcondes requested a review from a team as a code owner June 9, 2020 12:44
@cauemarcondes cauemarcondes requested a review from a team June 9, 2020 12:44
@cauemarcondes cauemarcondes requested review from a team as code owners June 9, 2020 12:44
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 9, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@cauemarcondes
Copy link
Contributor Author

retest

@weltenwort weltenwort self-requested a review June 9, 2020 14:27
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment on the APM side of things, but this looks like a good start. For the data fetching to work, though, the service also needs to expose a way for the solutions to register custom context providers, which make data access available within their respective data fetcher functions.

x-pack/plugins/observability/typings/chart/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/service.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/service.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/service.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/plugin.ts Outdated Show resolved Hide resolved
@cauemarcondes cauemarcondes requested a review from weltenwort June 9, 2020 17:06
@cauemarcondes
Copy link
Contributor Author

retest

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're getting close 📈 I really like the symmetric substructure of the obs data service 👍 I just left a few suggestions about making its API more consistent below.

x-pack/plugins/observability/public/data_access_service.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/typings/chart/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/typings/chart/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/plugin.ts Outdated Show resolved Hide resolved
@jasonrhodes
Copy link
Member

  • I was wondering to have this name pre-defined on Observability like apm, logs, uptime, metrics, WDYT?

Can we bump this type up and export/share it in all the places we need to use these names for shared interfaces between the apps? https://github.com/elastic/kibana/blob/master/x-pack/plugins/observability/public/hooks/use_track_metric.tsx#L20

@jasonrhodes
Copy link
Member

jasonrhodes commented Jun 10, 2020

  • context: is any information you want to inject to the function that will be executed

I thought "context" was provided by Kibana core, with pre-scoped clients etc similar to how HTTP routing handlers work. If we are providing the values for context, do we know exactly what values we are going to provide so that all apps have what they need to make their calls?

This part is still really fuzzy to me.

@cauemarcondes
Copy link
Contributor Author

f we are providing the values for context, do we know exactly what values we are going to provide so that all apps have what they need to make their calls?

Maybe I misunderstand your question, but each plugin should define its own context. The Observability itself won't add any information.
e.g:

// plugin.ts
public setup(core: CoreSetup, plugins: ApmPluginSetupDeps) {
  plugins.observability.dataAccess.registerContext(
      this.initializerContext.opaqueId,
      'createCallApmApi',
      () => createCallApmApi
  );
  plugins.observability.dataAccess.registerProvider(
      this.initializerContext.opaqueId,
      'apm',
      getObservabilityChartData
  );
}

@jasonrhodes
Copy link
Member

jasonrhodes commented Jun 10, 2020

but each plugin should define its own context

Got it, so I'm trying to wrap my head around these layers of abstraction. How are these two things different:

1:

import { callApi } from '../callApi';

const getChartData = (context, { start, end }) => {
  context.callApi({ start, end, otherArgs });
}

plugins.observability.dataAccess.registerContext(
  this.initializerContext.opaqueId,
  'callApi',
  () => callApi
);
plugins.observability.dataAccess.registerProvider(
  this.initializerContext.opaqueId,
  'apm',
  getChartData
);

vs

2:

import { callApi } from '../callApi';

const getChartData = (context, { start, end }) => {
  callApi({ start, end, otherArgs });
}

plugins.observability.dataAccess.registerProvider(
  this.initializerContext.opaqueId,
  'apm',
  getChartData
);

@cauemarcondes
Copy link
Contributor Author

cauemarcondes commented Jun 10, 2020

but each plugin should define its own context

Got it, so I'm trying to wrap my head around these layers of abstraction. How are these two things different:

Conceptually, none. In both cases, you'll have callApi available, one through the context and another with closure.

But what if your getChartData needs to use some information available in another plugin lets say security ? You still could use closure to solve it, but you'd need to keep the function in the same scope where the licensing plugin is available.

public setup(core: CoreSetup, plugins: ApmPluginSetupDeps) {
  plugins.observability.dataAccess.registerProvider(
        this.initializerContext.opaqueId,
        'apm',
        () => {
          // do anything cool
          const isSecurityEnabled = plugins.security?.license.isEnabled();
        }
      );
}

And if you want to isolate your function in another file, you wouldn't have the security plugin in scope, so you'd do:

//plugin.ts
public setup(core: CoreSetup, plugins: ApmPluginSetupDeps) {
  plugins.observability.dataAccess.requiresAppContext(
        this.initializerContext.opaqueId,
        () => ({
          security: plugins.security,
        })
      );
  plugins.observability.dataAccess.registerProvider(
        this.initializerContext.opaqueId,
        'apm',
       getChartData
      );
}

//get_chart_data.ts
const getChartData = (context, { start, end }) => {
  const isSecurityEnabled = context.security?.license.isEnabled(); 
  // do anything cool
}

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@jasonrhodes
Copy link
Member

But what if your getChartData needs to use some information available in another plugin lets say security ? You still could use closure to solve it, but you'd need to keep the function in the same scope where the licensing plugin is available.

Right, ok. And if this function was being used in your own plugin, you could pass it the things it needs directly or using some React context/hook combination. The alternative would be to have the observability plugin call each registered function with a set of commonly needed interfaces, e.g. licensing, security, es client, etc and then the registered methods would use them as passed in.

I can see the advantage of defining your own context giving you the ultimate flexibility in what your registered function can do, but it seems to come at the expense of making everything much harder to follow. I'm curious if anyone else has thoughts on this pattern?

@jasonrhodes
Copy link
Member

Another thought: at the very least if each calling plugin need to pre-register its own function's context value, could it at least happen inside of registerProvider? Something like:

plugins.observability.dataAccess.registerProvider({
  opaqueId: this.initializerContext.opaqueId,
  name: 'apm',
  handler: getChartData,
  providedContext: { // or just context, whatever
    getData: () => {}, // all provided values here
  }
});

I'm not sure how much control over this API we have vs what comes from the registry context tooling we are using from core, but I think that at least might make it a little clearer how these things are related...

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for a first pass. I share some of @jasonrhodes concerns about the indirection and complexity, but don't have any constructive feedback for how we would do it differently. This is a great place to start and evolve from as needs change and we have experience using this in our plugins.

@cauemarcondes
Copy link
Contributor Author

The alternative would be to have the observability plugin call each registered function with a set of commonly needed interfaces, e.g. licensing, security, es client, etc

We can do it. I can pre-register some plugins in advance.

at the very least if each calling plugin need to pre-register its own function's context value, could it at least happen inside of registerProvider?

I will create an example as you propose.

@cauemarcondes
Copy link
Contributor Author

@jasonrhodes I made the changes as you suggested, it looks simpler in my opinion. Take a look here 0fcbcd2

plugins.observability.dataAccess.registerProvider({
  pluginOpaqueId: this.initializerContext.opaqueId,
  dataType: 'apm',
  handler: getObservabilityChartData,
  providedContext: {
    licensing: plugins.licensing,
    home: plugins.home,
  },
});

Then to access it in the handler function you'd do context.[dataType].[contextName].

WDYT?

@jasonrhodes
Copy link
Member

I created Observability own registry for the sake of comparison

Cool so if I understand correctly, in this case we'd need to explicitly type the contract that the obs plugin provides when it calls these dataFetcher callbacks, right? Do we know what that contract would need to look like?

@weltenwort
Copy link
Member

Are plugins using this registry provider outside of core? I think I heard that alerting uses it in some way? I think its apparent lack of documentation/examples is another thing that’s made it hard to follow or grok (maybe I’m missing those?).

A quick grep shows it's used outside of core by the data plugin to register search strategies.

Inside core it's also used by...

  • the application service to register applications.
  • the http plugin to register routes.

I'm surprised the number is so low. I would have expected things like the task manager (and thereby alerting), visualizations, expression functions and so on to use it to for their registries, but they implement custom mechanisms. Maybe because they predate the context service?

I think our use case is a perfect match, but if it turns out that not using it is simpler and easier I can follow that argument.

@weltenwort
Copy link
Member

I created Observability own registry for the sake of comparison.

What's the argument against coupling this to the obs plugin life-cycle? An implicit singleton seem unnecessarily hacky. I hope we're not conflating this with the context container topic. Those are different aspects.

@cauemarcondes
Copy link
Contributor Author

I created Observability own registry for the sake of comparison

Cool so if I understand correctly, in this case we'd need to explicitly type the contract that the obs plugin provides when it calls these dataFetcher callbacks, right? Do we know what that contract would need to look like?

@jasonrhodes I believe this is what you want:

export type DataFetcher = (searchParams: SearchParams) => Promise<FetcherResponse[]>;

@jasonrhodes
Copy link
Member

@cauemarcondes ok so if a dataFetcher needs access to e.g. the security plugin, etc. it will need to manage that itself via a closure when it registers the fetcher?

I thought we were going to explore providing a contract of core APIs at call time, but that's less flexible than letting the apps manage that for themselves, so I'm okay with either.

@cauemarcondes
Copy link
Contributor Author

@cauemarcondes ok so if a dataFetcher needs access to e.g. the security plugin, etc. it will need to manage that itself via a closure when it registers the fetcher?

Correct.

@sorenlouv
Copy link
Member

The context container approach is overly complex for what we need imo. If we need the additional complexity down the road we can always add it. But let's start simple if possible.

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM as a great starting point, thanks @cauemarcondes! I think we will probably realize we are all sharing enough of the same deps that we may eventually provide them via the FetchDataParams as a common contract/interface, but we can definitely add that later once we see how each solution is building its fetch data function.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 1610 -1 1611
infra 1610 -1 1611
observability 19 -1 20
uptime 1409 -1 1410
total - -4 -

page load asset size

beta
id value diff baseline
/bundles/plugin/observability/observability.plugin.js 102.9KB +561.0B 102.4KB

History

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

@cauemarcondes cauemarcondes merged commit 437f9f6 into elastic:master Jun 15, 2020
@cauemarcondes cauemarcondes deleted the obs-registry-context branch June 15, 2020 06:46
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Jun 15, 2020
…in registry function (elastic#68642)

* creating observability context registry

* adding registryContext

* addressing PR comments

* addressing PR comments

* adding context to registry provider

* adding obs own registry

* refactoring

* refactoring

* fixing types

* removing apm code

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 15, 2020
* master: (91 commits)
  [Search][BUG] Call wrong search strategy recursively in async search (elastic#69116)
  [Observability] Create context container to enable Observability plugin registry function (elastic#68642)
  Rename space id for disabled index pattern test (elastic#68990)
  skip flaky suite (elastic#63339)
  Resolver Light Theme And Kibana Integration (elastic#67859)
  [kbn/dev-utils] expose public tooling_log module (elastic#68868)
  index pattern(s) take dependencies as object (elastic#69055)
  include ci-stats metrics in pr comment (elastic#68563)
  Bump webpack packages (elastic#68716)
  [Uptime] Fixed metric query broken because of missing mapping (elastic#68999)
  Added cloud as an optional dependency (elastic#69050)
  Fixed all external links (elastic#68614)
  [DOCS] Reorganizes doc nav to match new Kibana nav (elastic#69069)
  [Endpoint] Using the stats provided by the backend for resolver UI (elastic#68577)
  [DOCS] Removees 8.0 from Upgrade Assistant docs (elastic#69067)
  [ML] Fix cloud deployment ID check (elastic#68695)
  [DOCS] Move metrics app content to metrics monitoring guide (elastic#69033)
  Add ingest manager topic to docs (elastic#68980)
  [SECURITY SOLUTION] EMT-401: add policy data to metadata and fix tests (elastic#68582)
  [DOCS] Fixes POST request for saved objects (elastic#69036)
  ...
cauemarcondes added a commit that referenced this pull request Jun 15, 2020
…in registry function (#68642) (#69125)

* creating observability context registry

* adding registryContext

* addressing PR comments

* addressing PR comments

* adding context to registry provider

* adding obs own registry

* refactoring

* refactoring

* fixing types

* removing apm code

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 15, 2020
* master: (60 commits)
  Re-enable mistakenly skipped tests. (elastic#69123)
  [Search][BUG] Call wrong search strategy recursively in async search (elastic#69116)
  [Observability] Create context container to enable Observability plugin registry function (elastic#68642)
  Rename space id for disabled index pattern test (elastic#68990)
  skip flaky suite (elastic#63339)
  Resolver Light Theme And Kibana Integration (elastic#67859)
  [kbn/dev-utils] expose public tooling_log module (elastic#68868)
  index pattern(s) take dependencies as object (elastic#69055)
  include ci-stats metrics in pr comment (elastic#68563)
  Bump webpack packages (elastic#68716)
  [Uptime] Fixed metric query broken because of missing mapping (elastic#68999)
  Added cloud as an optional dependency (elastic#69050)
  Fixed all external links (elastic#68614)
  [DOCS] Reorganizes doc nav to match new Kibana nav (elastic#69069)
  [Endpoint] Using the stats provided by the backend for resolver UI (elastic#68577)
  [DOCS] Removees 8.0 from Upgrade Assistant docs (elastic#69067)
  [ML] Fix cloud deployment ID check (elastic#68695)
  [DOCS] Move metrics app content to metrics monitoring guide (elastic#69033)
  Add ingest manager topic to docs (elastic#68980)
  [SECURITY SOLUTION] EMT-401: add policy data to metadata and fix tests (elastic#68582)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Observability Landing - Milestone 1 release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Observability] Create context container to enable Observability plugin registry function
7 participants