Instrument vis_type_vislib, lens and vis_type_timeseries with execution context service #105206

mshustov · 2021-07-12T11:51:07Z

Summary

Part of #101587
It integrates a few visualizations with a mechanism allowing users to understand what Kibana entity initiated a search request.
It can be used to identify a source of slowness via Elasticsearch slow log query: the slow log record contains x-opaque-id header, which value is propagated from the Kibana server or browser App.

The current PR integrates the execution_context service with :

vis_type_vislib

kibana/src/plugins/vis_type_vislib/public/types.ts

Lines 39 to 50 in cc41571

    
           export const VislibChartType = Object.freeze({ 
        
             Histogram: 'histogram' as const, 
        
             HorizontalBar: 'horizontal_bar' as const, 
        
             Line: 'line' as const, 
        
             Pie: 'pie' as const, 
        
             Area: 'area' as const, 
        
             PointSeries: 'point_series' as const, 
        
             Heatmap: 'heatmap' as const, 
        
             Gauge: 'gauge' as const, 
        
             Goal: 'goal' as const, 
        
             Metric: 'metric' as const, 
        
           });

lens
vis_type_timeseries

I'd appreciate any suggestion from the team-owners on improving the logic or general guidance on the execution_context service integration.

Implementation

The browser environment doesn't provide an API to keep track of the "thread" context during async operations.
In nodejs env, we rely on AsyncLocalStorage and Async hooks for this purpose.
Thus, for the browser env, we have to implement an alternative approach. A Kibana plugin creates an async context object and passes it manually through the whole invocation chain to a place when the Kibana browser app communicates with the Kibana server via an HTTP protocol. Then the propagated async context object is serialized and passed through the network to the Kibana server. In turn, the Kibana server stores it as a part of the "thread" context, emits async context object to the logs, and attaches id and type of async context object as part of x-opaque-id header to every call to the Elasticsearch server. Therefore, a Kibana user can identify what a feature initiated a request via Kibana logs (under elasticsearch.query logger) or Elasticsearch logs (x-opaque-id is attached to search slow logs).

How to test

Unfortunately, right at this moment, only manually. I'm going to add integration tests in a follow-up PR.
Steps to test the PR manually:

setup logging to observe request.id: opaqueId in elasticsearch response and execution_context whenever it's set

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-07-12T13:14:28.944+03:00][DEBUG][execution_context] stored the execution context: {"requestId":"d568e29f-c3de-4ef6-a311-225b1358f8b7","type":"visuzalization","name":"gauge","description":"[eCommerce] Average Sales Price","id":"4b3ec120-b892-11e8-a6d9-e546fe2bba5f","url":"/bon/app/visualize#/edit/4b3ec120-b892-11e8-a6d9-e546fe2bba5f"}

2021-07-12T13:14:28.965+03:00|{"http":{"request":{"id":"d568e29f-c3de-4ef6-a311-225b1358f8b7;kibana:visuzalization:gauge:4b3ec120-b892-11e8-a6d9-e546fe2bba5f"}}}

mshustov · 2021-07-12T11:53:14Z

src/core/server/elasticsearch/client/configure_client.test.ts

@@ -146,6 +153,7 @@ describe('configureClient', () => {
                      "200
                  GET /foo?hello=dolly
                  {\\"seq_no_primary_term\\":true,\\"query\\":{\\"term\\":{\\"user\\":\\"kimchy\\"}}}",
+                      undefined,


I decided to update the test instead of refactoring.

mshustov · 2021-07-12T11:54:33Z

src/core/server/elasticsearch/client/configure_client.ts

+        ? {
+            http: { request: { id: event.meta.request.options.opaqueId } },
+          }
+        : undefined; // do not clutter logs if opaqueId is not present


"request": { } adds noise to the logs and consumes more disk space, but doesn't bring any value.

mshustov · 2021-07-12T11:57:23Z

src/core/server/elasticsearch/client/configure_client.ts

+      const opaqueId = event.meta.request.options.opaqueId;
+      const meta = opaqueId
+        ? {
+            http: { request: { id: event.meta.request.options.opaqueId } },


we will remove this logic when we start including trace.id in the logs. #102699
Instead, trace.id will be used for the log correlation. But right now we need it for testing.

mshustov · 2021-07-12T12:05:45Z

src/core/types/execution_context.ts

-export interface KibanaExecutionContext {
+// to make it compatible with SerializableState
+// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
+export type KibanaExecutionContext = {


I used to get is incompatible with index signature error when declared it as interface.
We cannot use KibanaExecutionContext extends SerializableState since Core cannot depend on the Kibana plugins.

i think this is the relevant typescript issue: microsoft/TypeScript#15300

mshustov · 2021-07-12T12:19:10Z

src/plugins/data/public/search/expressions/esaggs.ts

@@ -58,6 +62,7 @@ export function getFunctionDefinition({
        timeFields: args.timeFields,
        timeRange: get(input, 'timeRange', undefined),
        getNow,
+        executionContext: executionContext?.toJSON(),


The data plugin data fetching model is a bit different from the one most plugins use. It doesn't send an HTTP request for every search operation, but batches them according to some internal rules and send them in bulk. In turn, the Kibana server parses the batch and issues a dedicated search request for every search operation. Kibana server streams Elasticsearch server response back to the browser as soon as every search operation is finished.

So I had to refactor the plugin a bit to support executionContext propagation.

why are we passing json here ?

the downstream code requires SerializableState, so I had to convert to json before calling fetch$

SerializableState is any object that can be serialized, seems KibanaExecutionContext already is

KibanaExecutionContext is serializable, but the handler receives https://github.com/elastic/kibana/pull/105206/files#diff-357bbbe4bfbe5a09c1e453b99baba2082a43ef48d34d1677831cc3c485f1bca6R70

is there a way to make it serializable without converting to json string ?

is there a way to make it serializable without converting to json string ?

It's not a json string, it returns a json object. Actually, any object with toJSON method is serializable with JSON.stringify, but SerializableState interface doesn't support it.

mshustov · 2021-07-12T12:19:54Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+      executionContext: this.deps.start().core.executionContext.create({
+        type: this.vis.params.type,
+        name: this.vis.type.name,
+        id: this.vis.id ?? 'unknown_id',


In what cases it may be empty? What should we use instead?

unsaved visualization

embeddable shouldn't be creating the execution context (as else we don't know if request came from dashboard or visualize for example) it should be the app that is responsible for this.

we should add execution context to EmbeddableInput (just as we did with searchSessionId)

as discussed in person, we will add this later. see Nested context in #102629

mshustov · 2021-07-12T12:21:17Z

x-pack/plugins/lens/public/embeddable/embeddable.tsx

+      name: this.savedVis.visualizationType ?? '',
+      description: this.savedVis.title ?? this.savedVis.description ?? '',
+      id: this.id,
+      url: this.output.editUrl,


I saw url: "app/lens#/edit_by_value" during the testing. What url should I use instead?

that's probably correct url, other state is passed thru navigateToAppwithstate

For the "by-value" Lens panels we can't provide a linkable URL yet. Is there a way we could link to the dashboard URL instead, as the dashboard is has a URL?

Is there a way we could link to the dashboard URL instead, as the dashboard is has a URL?

not yet. in #102626 I'm going to add nested context support, so we can create and link dashboard context and visualization context.

elasticmachine · 2021-07-12T12:59:00Z

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

ppisljar · 2021-07-14T11:35:18Z

ignore my comment about using search sessions for this i don't think its gonna be helpful

wylieconlon

Tested according to your instructions and found that it was working well. My main comments are about the actual values that you've chosen for Lens & Visualize executionContext.

wylieconlon · 2021-07-14T16:54:10Z

docs/development/core/server/kibana-plugin-core-server.kibanaexecutioncontext.md

-|  [id](./kibana-plugin-core-server.kibanaexecutioncontext.id.md) | <code>string</code> | unique value to indentify find the source |
-|  [name](./kibana-plugin-core-server.kibanaexecutioncontext.name.md) | <code>string</code> | public name of a user-facing feature |
-|  [type](./kibana-plugin-core-server.kibanaexecutioncontext.type.md) | <code>string</code> | Kibana application initated an operation. Can be narrowed to an enum later. |
-|  [url](./kibana-plugin-core-server.kibanaexecutioncontext.url.md) | <code>string</code> | in browser - url to navigate to a current page, on server - endpoint path, for task: task SO url |


@mshustov It looks like the generated docs have lost some comments- can you bring them back?

Hm..it's how our doc generator handles interface conversion into type. Let me see what I can do with it.

wylieconlon · 2021-07-14T17:02:07Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+        type: this.vis.params.type,
+        name: this.vis.type.name,


Both of these values are the same in practice, is that intentional? Seems like if they are going to be the same value they should use the same accessor.

I was going to use type: visualization, it might be useful for cases like lens or actions when we need an additional sub-type to identify a source.

wylieconlon · 2021-07-14T17:03:19Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+        type: this.vis.params.type,
+        name: this.vis.type.name,
+        id: this.vis.id ?? 'unknown_id',
+        description: this.vis.title ?? this.vis.type.title,


Maybe the title should be used as name and this.vis.description should be used as the description?

name is documented as the public name of a user-facing feature so I think providing the name of the type of visualization better maps to that field. description is better suited for the specific instance of the feature, like the specific visualization title.

x-pack/plugins/lens/public/embeddable/embeddable.tsx

wylieconlon · 2021-07-14T17:06:11Z

x-pack/plugins/lens/public/embeddable/embeddable.tsx

@@ -323,7 +325,15 @@ export class Embeddable
    if (this.input.onLoad) {
      this.input.onLoad(true);
    }
+    const executionContext = this.deps.executionContext.create({
+      type: this.savedVis.type ?? 'lens',
+      name: this.savedVis.visualizationType ?? '',


So you want to track it as name: 'lnsPie' for example? It's not a unique name.

@wylieconlon Yes, id and description might be unique, but name is a public-facing name of the Kibana entity.

Seems like maybe we could make this parameter name more explicit in a follow-up. Maybe this would be more clear:

type renamed to feature

name renamed to featureSubtype

description renamed to instanceDescription

id renamed to instanceId

@mshustov WDYT?

wylieconlon · 2021-07-14T17:07:04Z

x-pack/plugins/lens/public/embeddable/embeddable.tsx

+      name: this.savedVis.visualizationType ?? '',
+      description: this.savedVis.title ?? this.savedVis.description ?? '',
+      id: this.id,
+      url: this.output.editUrl,


For the "by-value" Lens panels we can't provide a linkable URL yet. Is there a way we could link to the dashboard URL instead, as the dashboard is has a URL?

TinaHeiligers · 2021-07-16T00:00:30Z

src/core/public/index.ts


 export type { MountPoint, UnmountCallback, PublicUiSettingsParams } from './types';

 export { URL_MAX_LENGTH } from './core_app';

+export type { KibanaExecutionContext } from './execution_context';


Why is the additional type export from the same location needed?

joshdover

Just one test that I think needs to be adjusted, but otherwise LGTM. I'll push up the change to that test while @mshustov is on PTO. We can merge once we get @ppisljar's ✅

src/core/server/execution_context/execution_context_container.test.ts

joshdover · 2021-07-16T11:15:18Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+        type: this.vis.params.type,
+        name: this.vis.type.name,
+        id: this.vis.id ?? 'unknown_id',
+        description: this.vis.title ?? this.vis.type.title,


name is documented as the public name of a user-facing feature so I think providing the name of the type of visualization better maps to that field. description is better suited for the specific instance of the feature, like the specific visualization title.

ppisljar

code LGTM

kibanamachine · 2021-07-19T13:28:26Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`core`	1071	1072	+1
`data`	3278	3281	+3
`expressions`	1565	1567	+2
total			+6

Async chunks

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

id	before	after	diff
`lens`	1.5MB	1.5MB	+557.0B
`visualizations`	101.1KB	101.4KB	+373.0B
total			+930.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`	421.6KB	421.7KB	+96.0B
`data`	843.2KB	843.6KB	+380.0B
`expressions`	213.8KB	213.9KB	+152.0B
`lens`	28.8KB	28.9KB	+112.0B
`visTypeTimeseries`	24.5KB	24.7KB	+138.0B
total			+878.0B

Unknown metric groups

API count

id	before	after	diff
`core`	2328	2329	+1
`data`	3844	3847	+3
`expressions`	1998	2003	+5
total			+9

History

💚 Build #139028 succeeded 166a4e1
💚 Build #138770 succeeded e0a2ddf
💚 Build #138725 succeeded c8f3752
💔 Build #138763 failed 859caf2
💚 Build #138469 succeeded 58a3886

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

cc @joshdover

…on context service (elastic#105206) Co-authored-by: Josh Dover <1813008+joshdover@users.noreply.github.com>

kibanamachine · 2021-07-19T15:38:12Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…on context service (#105206) (#106117) Co-authored-by: Josh Dover <1813008+joshdover@users.noreply.github.com> Co-authored-by: Mikhail Shustov <restrry@gmail.com> Co-authored-by: Josh Dover <1813008+joshdover@users.noreply.github.com>

mshustov added 11 commits July 8, 2021 13:03

export executionContext types

e79b36c

pass execution context for expressions and bsearch

9f80622

instrument lens

00ae806

remove duplicated imports

105bc80

fix type

36891cd

instrument tsvb

73e4be0

pass data as container

70c070e

attach request.id to elasticsearch response logs

1def5a3

emits execution context to the debug logs

09333ba

update es client test

9cb03e4

attach url to the execution context

cc41571

mshustov added 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 Jul 12, 2021

mshustov commented Jul 12, 2021

View reviewed changes

update executionContext mock

3bedbb6

mshustov commented Jul 12, 2021

View reviewed changes

mshustov added 2 commits July 12, 2021 15:41

fix type errors in tests

b1efd7f

update docs

628227d

mshustov marked this pull request as ready for review July 12, 2021 12:58

mshustov requested a review from a team July 12, 2021 12:58

mshustov requested review from a team as code owners July 12, 2021 12:58

mshustov added 2 commits July 14, 2021 16:31

add comment and mark readonly

e9e9ca0

remove leftovers of an alternative approach

e432f3b

mshustov marked this pull request as ready for review July 14, 2021 14:25

update docs

58a3886

mshustov requested a review from ppisljar July 14, 2021 14:32

wylieconlon approved these changes Jul 14, 2021

View reviewed changes

mshustov added 5 commits July 15, 2021 09:05

Merge branch 'master' into inspect-vis

944ce6d

include name in x-opaque-id header

c8f3752

make expressions executionContext getter

6011737

but interface back for ExecutionContext type

859caf2

update docs

e0a2ddf

TinaHeiligers reviewed Jul 16, 2021

View reviewed changes

joshdover self-assigned this Jul 16, 2021

joshdover approved these changes Jul 16, 2021

View reviewed changes

joshdover added 3 commits July 16, 2021 13:27

Merge remote-tracking branch 'upstream/master' into inspect-vis

997c63e

Update unicode encoding test

166a4e1

Merge remote-tracking branch 'upstream/master' into inspect-vis

35c041d

ppisljar approved these changes Jul 19, 2021

View reviewed changes

joshdover added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 19, 2021

joshdover merged commit f9089e1 into elastic:master Jul 19, 2021

kibanamachine mentioned this pull request Jul 19, 2021

[7.x] Instrument vis_type_vislib, lens and vis_type_timeseries with execution context service (#105206) #106117

Merged

mshustov deleted the inspect-vis branch July 29, 2021 08:02

mshustov mentioned this pull request Aug 3, 2021

Implement execution context management service #102626

Closed

2 tasks

lukasolson mentioned this pull request Nov 10, 2021

[data.search] Move integration with execution context to data plugin search service #118230

Closed

thomasneirynck mentioned this pull request Dec 13, 2021

[Maps] Add execution_context #121116

Closed

	export const VislibChartType = Object.freeze({
	Histogram: 'histogram' as const,
	HorizontalBar: 'horizontal_bar' as const,
	Line: 'line' as const,
	Pie: 'pie' as const,
	Area: 'area' as const,
	PointSeries: 'point_series' as const,
	Heatmap: 'heatmap' as const,
	Gauge: 'gauge' as const,
	Goal: 'goal' as const,
	Metric: 'metric' as const,
	});

Instrument vis_type_vislib, lens and vis_type_timeseries with execution context service #105206

Instrument vis_type_vislib, lens and vis_type_timeseries with execution context service #105206

Conversation

mshustov commented Jul 12, 2021 • edited Loading

Summary

Implementation

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 12, 2021

ppisljar commented Jul 14, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 19, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

kibanamachine commented Jul 19, 2021

💚 Backport successful

mshustov commented Jul 12, 2021 •

edited

Loading

mshustov Jul 15, 2021 •

edited

Loading

mshustov Jul 15, 2021 •

edited

Loading