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

[Execution context] Add nested context support #107523

Merged
merged 12 commits into from
Aug 10, 2021

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Aug 3, 2021

Summary

Part of #102626

Adds support for declaring parent-child relationships between execution contexts. With this change, Kibana might log and pass to Elasticsearch information about the origin of every spawned execution context.
kibana logs

{
   "parent":{
      "type":"application",
      "name":"dashboard",
      "id":"722b74f0-b882-11e8-a6d9-e546fe2bba5f",
      "description":"[eCommerce] Revenue Dashboard",
      "url":"/view/722b74f0-b882-11e8-a6d9-e546fe2bba5f"
   },
   "type":"visualization",
   "name":"metrics",
   "id":"45e07720-b890-11e8-a6d9-e546fe2bba5f",
   "description":"[eCommerce] Promotion Tracking",
   "url":"/app/visualize#/edit/45e07720-b890-11e8-a6d9-e546fe2bba5f"
}

x-opaque-id header

75221494-b6fb-40e2-a392-fdab0f500eb8;kibana:application:dashboard:722b74f0-b882-11e8-a6d9-e546fe2bba5f;visualization:metrics:45e07720-b890-11e8-a6d9-e546fe2bba5f

Client-side API

Due to lack of native JS API, we have to pass a parent context during instantiation of a child context:

// in the parent app
const parentContext = {
  type: 'application',
  name: 'dashboard',
  ...
});

createChild(parentContext);

// in a child app
const context = executionContext.create({
  parent: parentContext,
  type: 'vis',
  ...
});

Note that EmbeddableInput is expected to be compatible with SerializableState interface, so I had to remove IExecutionContextContainer usage in favor of KibanaExecutionContext which is POJO. With this change, using execution_context service on the client-side looks like overhead, so I removed it completely.

Server-side API

On the server-side, plugins may register execution context by wrapping any function in withContext. Provided context object is carried over all async operations spawned by the passed function. Sequentual calls withContext will create a stack of context.

// in the parent app
executionContext.withContext({
  type: 'task',
  name: 'my-task',
  ...
}, async () => {
  return await fetchData();
});

How to test

logging:
  appenders:
    myconsole:
      type: console
      layout:
        type: pattern
        pattern: '%date|%meta'
  loggers:
    - name: elasticsearch.query
      level: all
      appenders: [myconsole]
    - name: elasticsearch.query.taskManager
      level: off
    - name: execution_context
      level: debug
      appenders: [console]
  • run Kibana
  • navigate to Kibana Browser App
  • Install sample data. I tested on Sample eCommerce orders.
  • Navigate to dashboards.
  • You should be able to see the output like this:
[2021-08-03T15:30:58.891+03:00][DEBUG][execution_context] set the execution context: {"parent":{"type":"application","name":"dashboard","id":"722b74f0-b882-11e8-a6d9-e546fe2bba5f","description":"[eCommerce] Revenue Dashboard","url":"/view/722b74f0-b882-11e8-a6d9-e546fe2bba5f"},"type":"visualization","name":"metrics","id":"45e07720-b890-11e8-a6d9-e546fe2bba5f","description":"[eCommerce] Promotion Tracking","url":"/app/visualize#/edit/45e07720-b890-11e8-a6d9-e546fe2bba5f"}

2021-08-03T15:30:58.899+03:00|{"http":{"request":{"id":"75221494-b6fb-40e2-a392-fdab0f500eb8;kibana:application:dashboard:722b74f0-b882-11e8-a6d9-e546fe2bba5f;visualization:metrics:45e07720-b890-11e8-a6d9-e546fe2bba5f"}}}

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Aug 3, 2021
ExecutionContextContaier is not compatible with SerializableState, so I had to fall back to passing context as POJO. With this change, using a service looks like overhead.
@mshustov mshustov marked this pull request as ready for review August 4, 2021 04:46
@mshustov mshustov requested a review from a team August 4, 2021 04:46
@mshustov mshustov requested review from a team as code owners August 4, 2021 04:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

app services code LGTM

@botelastic botelastic bot added Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Aug 4, 2021
Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Dashboard changes look fine to me!

@joshdover
Copy link
Contributor

Wanted to get to this today but ran out of time, will do tomorrow - Aug 6

src/core/public/execution_context/index.ts Outdated Show resolved Hide resolved
@@ -34,7 +34,7 @@ export interface IExecutionContextContainer {
}

export class ExecutionContextContainer implements IExecutionContextContainer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this class would make more sense to be named as ExecutionContextSerializer now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will have different abstractions on the client and server then. I'd keep it as is. Considering that it's an internal API, we can always rename it later.

src/core/public/public.api.md Outdated Show resolved Hide resolved
description: 'new-description',
};
router.get({ path: '/execution-context', validate: false }, async (context, req, res) => {
const { headers } = await executionContext.withContext(newContext, () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we provide executionContext.withContext on RequestHandlerContext too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd wait until the first use case.
I don't think lots of endpoints will use this API manually to create a domain-specific context. For most of the cases, instrumentation performed by Core will be enough.

name: 'name-b',
id: 'id-b',
description: 'description-b',
parent: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if this is the behavior we actually want instead of subsequent calls using the previous context as the parent. If pluginA calls a sync API on pluginB and they both set context, wouldn't we want to track that relationship?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If pluginA calls a sync API on pluginB and they both set context, wouldn't we want to track that relationship?

set is Core-specific API which won't be provided to plugins. withContext which is exposed to plugins, supports context stacking

it('supports nested contexts', async () => {

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

KibanaApp owned code LGTM, tested adding TSVB + Lens visualization to Dashboard, since embeddable code was modified, works as expected

@mshustov mshustov requested a review from joshdover August 9, 2021 09:39
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM

@mshustov
Copy link
Contributor Author

mshustov commented Aug 9, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
core 368 367 -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 221.5KB 221.9KB +413.0B
lens 1.6MB 1.6MB -149.0B
visualizations 101.8KB 101.8KB +29.0B
total +293.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 423.9KB 423.4KB -513.0B
dashboard 329.2KB 329.3KB +68.0B
data 791.3KB 791.2KB -132.0B
lens 29.1KB 29.0KB -112.0B
total -689.0B
Unknown metric groups

API count

id before after diff
core 2455 2436 -19
dashboard 160 161 +1
total -18

API count missing comments

id before after diff
core 1166 1160 -6
dashboard 137 138 +1
total -5

History

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

@mshustov mshustov merged commit 5480c4d into elastic:master Aug 10, 2021
@mshustov mshustov deleted the issue-102626-nested-context branch August 10, 2021 06:53
mshustov added a commit to mshustov/kibana that referenced this pull request Aug 10, 2021
* Add nested context support

* remove execution context service on the client side

ExecutionContextContaier is not compatible with SerializableState, so I had to fall back to passing context as POJO. With this change, using a service looks like overhead.

* update docs

* fix test

* address comments from Josh

* put export back

* update docs

* remove outdated export

* use input.title for unsaved vis

Co-authored-by: Kibana Machine <[email protected]>
mshustov added a commit that referenced this pull request Aug 10, 2021
* Add nested context support

* remove execution context service on the client side

ExecutionContextContaier is not compatible with SerializableState, so I had to fall back to passing context as POJO. With this change, using a service looks like overhead.

* update docs

* fix test

* address comments from Josh

* put export back

* update docs

* remove outdated export

* use input.title for unsaved vis

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

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants