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

[Step 3] VisEditors Telemetry enhancements (add new agg-based and lens telemetries) #135615

Merged
merged 61 commits into from
Jul 25, 2022
Merged
Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
88a4445
initial comit
alexwizp Jul 1, 2022
d162e8e
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Jul 1, 2022
f49fcdc
push chart_expressions logic
alexwizp Jul 4, 2022
85e72e4
update tests
alexwizp Jul 4, 2022
d8f22b7
fix JEST
alexwizp Jul 4, 2022
c19effd
push some telemetries
alexwizp Jul 4, 2022
bbe81ee
fix some cases
alexwizp Jul 4, 2022
2983c10
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Jul 4, 2022
5216d02
update tests
alexwizp Jul 5, 2022
5b45314
add some lens part
alexwizp Jul 5, 2022
e44ffaf
add handlers.logRenderTelemetry method
alexwizp Jul 5, 2022
6f19ac8
visGroup -> originatingApp
alexwizp Jul 5, 2022
06fc16a
remove visTpe, extra, onlyExtra
alexwizp Jul 5, 2022
889de52
remove handlers.logRenderTelemetry from handlers
alexwizp Jul 6, 2022
0a5e43d
remove context from snapshots
alexwizp Jul 6, 2022
0de8332
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Jul 6, 2022
202dbe6
add lens operations telemetry
alexwizp Jul 7, 2022
a5dae2e
fix heaatmap, vislib
alexwizp Jul 7, 2022
e44f8d9
push some telemetries
alexwizp Jul 7, 2022
66488c7
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 8, 2022
3effc43
Merge branch 'lens-emb' into telemetry_1
alexwizp Jul 8, 2022
0e35990
cleanup
alexwizp Jul 8, 2022
208e84b
push some logic
alexwizp Jul 8, 2022
7645cbe
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 11, 2022
0ab03db
fix merge conflicts
alexwizp Jul 11, 2022
268ed67
push some logic
alexwizp Jul 11, 2022
56f4f7e
add lens map telemetry
alexwizp Jul 11, 2022
733f9c0
add render_lens_vis_cases
alexwizp Jul 11, 2022
c19d736
add render_lens_vis_observability_exploratory_view
alexwizp Jul 11, 2022
ccdeb56
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 11, 2022
75cfe46
cleanup
alexwizp Jul 11, 2022
db05044
cleanup
alexwizp Jul 12, 2022
bec9aa4
make getRenderEventCounters optional
alexwizp Jul 12, 2022
9cf6c07
add summary_row and color_by_value telemetries
alexwizp Jul 12, 2022
4124ad1
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 13, 2022
5886603
try to fix double rendering
alexwizp Jul 13, 2022
5d81cfe
update xy telemetries
alexwizp Jul 13, 2022
e7cdca3
fix TSVB
alexwizp Jul 13, 2022
41c8303
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 14, 2022
5fd079b
fix lens
alexwizp Jul 14, 2022
5a2f920
fix Timelion
alexwizp Jul 14, 2022
ef8cc9b
Merge branch 'main' into telemetry_1
kibanamachine Jul 18, 2022
a1bd5f1
add mixed_xy telemetry
alexwizp Jul 18, 2022
758b4cf
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jul 18, 2022
8610240
Merge branch 'main' into telemetry_1
kibanamachine Jul 18, 2022
6f7148d
Update x-pack/plugins/observability/public/components/shared/explorat…
alexwizp Jul 18, 2022
7df1e45
Update x-pack/plugins/observability/public/components/shared/explorat…
alexwizp Jul 18, 2022
c4da22e
Update expression_renderer.tsx
alexwizp Jul 18, 2022
a9ef0c0
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 19, 2022
f1f5bf0
update originatingApp
alexwizp Jul 19, 2022
73c358e
Update expression_renderer.tsx
alexwizp Jul 20, 2022
c1933ce
add JEST for core changes
alexwizp Jul 20, 2022
afc03cd
Merge branch 'main' into telemetry_1
kibanamachine Jul 21, 2022
6338af3
Update plugin.ts
alexwizp Jul 21, 2022
50c2ba6
Update src/plugins/expressions/common/expression_renderers/types.ts
alexwizp Jul 21, 2022
7ba73e7
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 22, 2022
0c84779
fix PR comments
alexwizp Jul 22, 2022
47f964d
add renderComplete param to VisualizationContainer
alexwizp Jul 22, 2022
b2c2acd
fix mixed_xy issue
alexwizp Jul 25, 2022
4cc6034
Merge branch 'main' into telemetry_1
kibanamachine Jul 25, 2022
71cc2d0
Merge remote-tracking branch 'upstream/main' into telemetry_1
alexwizp Jul 25, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export type KibanaExecutionContext = {
readonly description?: string;
/** in browser - url to navigate to a current page, on server - endpoint path, for task: task SO url */
readonly url?: string;
/** Metadata attached to the field. An optional parameter that allows to describe the execution context in more detail. **/
readonly meta?: { [key: string]: string | number | boolean | undefined };
Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is the only core file modified in this PR, I'm assuming this addition is not used by our EC system. May I ask what the purpose of this new field is, exactly? How/Where are you using it?

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 have 2 telemetries for which we need to pass additional meta information. It's a viewMode and fullscreenMode. It this PR we set it in in dashboard/public/application/hooks/use_dashboard_app_state.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'm probably missing something here.

To my knowledge, apart from sending it to the server / ES for context tracking, the EC is used in 2 places:

  1. By APM to add some labels:

start.executionContext.context$.subscribe((c) => {
// We're using labels because we want the context to be indexed
// https://www.elastic.co/guide/en/apm/get-started/current/metadata.html
const apmContext = start.executionContext.getAsLabels();
this.apm?.addLabels(apmContext);
});

getAsLabels: () => {
return this.removeUndefined({
name: this.appId,
id: this.context$.value?.id,
page: this.context$.value?.page,
}) as Labels;
},

Which, without changes, is not using this new meta field

  1. For EBT to enrich the context

private enrichAnalyticsContext(analytics: AnalyticsServiceSetup) {
analytics.registerContextProvider({
name: 'execution_context',
context$: this.context$.pipe(
map(({ type, name, page, id }) => ({
pageName: `${compact([type, name, page]).join(':')}`,
applicationId: name ?? type ?? 'unknown',
page,
entityId: id,
}))
),

Which, without changes, is not using this new meta field either.

So, where exactly is this new field consumed? Is that in custom telemetry collectors?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pgayvallet For now we don't send it out in custom telemetry collectors but pick it from the context in the expression renderer and pass it to ui counters. Definitely possible we want to add such an integration at a later point though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was implemented here cause we need somehow to pass viewMode and fullscreenMode properties from the container (e.g. DashboardContainer) into Render expression function. Therefore, the main consumer of this is: lens/public/embeddable/embeddable.tsx (see getExecutionContextEvents method).

/** an inner context spawned from the current context. */
child?: KibanaExecutionContext;
};
2 changes: 1 addition & 1 deletion packages/kbn-optimizer/limits.yml
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,6 @@ pageLoadAssetSize:
eventAnnotation: 19334
screenshotting: 22870
synthetics: 40958
expressionXY: 34000
expressionXY: 36000
kibanaUsageCollection: 16463
kubernetesSecurity: 77234
2 changes: 1 addition & 1 deletion src/dev/storybook/aliases.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const storybookAliases = {
es_ui_shared: 'src/plugins/es_ui_shared/.storybook',
expression_error: 'src/plugins/expression_error/.storybook',
expression_image: 'src/plugins/expression_image/.storybook',
expression_metric_vis: 'src/plugins/chart_expressions/expression_metric/.storybook',
expression_metric_vis: 'src/plugins/chart_expressions/expression_legacy_metric/.storybook',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix regression of #135461

expression_metric: 'src/plugins/expression_metric/.storybook',
expression_partition_vis: 'src/plugins/chart_expressions/expression_partition_vis/.storybook',
expression_repeat_image: 'src/plugins/expression_repeat_image/.storybook',
Expand Down
9 changes: 9 additions & 0 deletions src/plugins/chart_expressions/common/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export { extractContainerType, extractVisualizationType } from './utils';
35 changes: 35 additions & 0 deletions src/plugins/chart_expressions/common/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import type { KibanaExecutionContext } from '@kbn/core-execution-context-common';

export const extractContainerType = (context?: KibanaExecutionContext): string | undefined => {
if (context) {
const recursiveGet = (item: KibanaExecutionContext): KibanaExecutionContext | undefined => {
if (item.type) {
return item;
} else if (item.child) {
return recursiveGet(item.child);
}
};
return recursiveGet(context)?.type;
}
};

export const extractVisualizationType = (context?: KibanaExecutionContext): string | undefined => {
if (context) {
const recursiveGet = (item: KibanaExecutionContext): KibanaExecutionContext | undefined => {
if (item.child) {
return recursiveGet(item.child);
} else {
return item;
}
};
return recursiveGet(context)?.type;
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
GaugeLabelMajorModes,
GaugeTicksPositions,
} from '../constants';
import { ExecutionContext } from '@kbn/expressions-plugin';

describe('interpreter/functions#gauge', () => {
const fn = functionWrapper(gaugeFunction());
Expand All @@ -39,7 +40,7 @@ describe('interpreter/functions#gauge', () => {
const checkArg = (arg: keyof GaugeArguments, options: Record<string, string>) => {
Object.values(options).forEach((option) => {
it(`returns an object with the correct structure for the ${option} ${arg}`, () => {
const actual = fn(context, { ...args, [arg]: option }, undefined);
const actual = fn(context, { ...args, [arg]: option });
expect(actual).toMatchSnapshot();
});
});
Expand All @@ -51,55 +52,43 @@ describe('interpreter/functions#gauge', () => {
checkArg('labelMajorMode', GaugeLabelMajorModes);

it(`returns an object with the correct structure for the circle if centralMajor and centralMajorMode are passed`, () => {
const actual = fn(
context,
{
...args,
shape: GaugeShapes.CIRCLE,
centralMajor: 'Some label',
centralMajorMode: GaugeCentralMajorModes.CUSTOM,
},
undefined
);
const actual = fn(context, {
...args,
shape: GaugeShapes.CIRCLE,
centralMajor: 'Some label',
centralMajorMode: GaugeCentralMajorModes.CUSTOM,
});
expect(actual).toMatchSnapshot();
});

it(`returns an object with the correct structure for the arc if centralMajor and centralMajorMode are passed`, () => {
const actual = fn(
context,
{
...args,
shape: GaugeShapes.ARC,
centralMajor: 'Some label',
centralMajorMode: GaugeCentralMajorModes.CUSTOM,
},
undefined
);
const actual = fn(context, {
...args,
shape: GaugeShapes.ARC,
centralMajor: 'Some label',
centralMajorMode: GaugeCentralMajorModes.CUSTOM,
});
expect(actual).toMatchSnapshot();
});

it(`throws error if centralMajor or centralMajorMode are provided for the horizontalBullet shape`, () => {
const actual = () =>
fn(
context,
{ ...args, centralMajor: 'Some label', centralMajorMode: GaugeCentralMajorModes.CUSTOM },
undefined
);
fn(context, {
...args,
centralMajor: 'Some label',
centralMajorMode: GaugeCentralMajorModes.CUSTOM,
});
expect(actual).toThrowErrorMatchingSnapshot();
});

it(`throws error if centralMajor or centralMajorMode are provided for the vertical shape`, () => {
const actual = () =>
fn(
context,
{
...args,
shape: GaugeShapes.VERTICAL_BULLET,
centralMajor: 'Some label',
centralMajorMode: GaugeCentralMajorModes.CUSTOM,
},
undefined
);
fn(context, {
...args,
shape: GaugeShapes.VERTICAL_BULLET,
centralMajor: 'Some label',
centralMajorMode: GaugeCentralMajorModes.CUSTOM,
});
expect(actual).toThrowErrorMatchingSnapshot();
});

Expand All @@ -114,8 +103,10 @@ describe('interpreter/functions#gauge', () => {
reset: () => {},
},
},
};
await fn(context, args, handlers as any);
getExecutionContext: jest.fn(),
} as unknown as ExecutionContext;

await fn(context, args, handlers);

expect(loggedTable!).toMatchSnapshot();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
GaugeColorModes,
GaugeCentralMajorModes,
} from '../constants';

export type GaugeColorMode = $Values<typeof GaugeColorModes>;
export type GaugeShape = $Values<typeof GaugeShapes>;
export type GaugeLabelMajorMode = $Values<typeof GaugeLabelMajorModes>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@
"ui": true,
"requiredPlugins": ["expressions", "fieldFormats", "charts", "visualizations", "presentationUtil", "data"],
"requiredBundles": ["kibanaUtils", "kibanaReact"],
"optionalPlugins": [],
"optionalPlugins": ["usageCollection"],
"extraPublicDirs": ["common"]
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,71 @@ import { i18n } from '@kbn/i18n';
import React from 'react';
import { render, unmountComponentAtNode } from 'react-dom';
import { PersistedState } from '@kbn/visualizations-plugin/public';
import { ThemeServiceStart } from '@kbn/core/public';
import { KibanaThemeProvider } from '@kbn/kibana-react-plugin/public';
import { ExpressionRenderDefinition } from '@kbn/expressions-plugin/common/expression_renderers';
import { EXPRESSION_GAUGE_NAME, GaugeExpressionProps } from '../../common';
import { getFormatService, getPaletteService, getThemeService } from '../services';
import { StartServicesGetter } from '@kbn/kibana-utils-plugin/public';
import { METRIC_TYPE } from '@kbn/analytics';
import { ExpressionGaugePluginStart } from '../plugin';
import { EXPRESSION_GAUGE_NAME, GaugeExpressionProps, GaugeShapes } from '../../common';
import { getFormatService, getPaletteService } from '../services';
import { extractContainerType, extractVisualizationType } from '../../../common';

interface ExpressionGaugeRendererDependencies {
theme: ThemeServiceStart;
getStartDeps: StartServicesGetter<ExpressionGaugePluginStart>;
}

export const gaugeRenderer: (
deps: ExpressionGaugeRendererDependencies
) => ExpressionRenderDefinition<GaugeExpressionProps> = ({ theme }) => ({
) => ExpressionRenderDefinition<GaugeExpressionProps> = ({ getStartDeps }) => ({
name: EXPRESSION_GAUGE_NAME,
displayName: i18n.translate('expressionGauge.renderer.visualizationName', {
defaultMessage: 'Gauge',
}),
reuseDomNode: true,
render: async (domNode, config, handlers) => {
const { core, plugins } = getStartDeps();

handlers.onDestroy(() => {
unmountComponentAtNode(domNode);
});

const renderComplete = () => {
let type: string;

switch (config.args.shape) {
case GaugeShapes.HORIZONTAL_BULLET:
type = `${EXPRESSION_GAUGE_NAME}_horizontal`;
break;
case GaugeShapes.VERTICAL_BULLET:
type = `${EXPRESSION_GAUGE_NAME}_vertical`;
break;
default:
type = EXPRESSION_GAUGE_NAME;
}

const executionContext = handlers.getExecutionContext();
const containerType = extractContainerType(executionContext);
const visualizationType = extractVisualizationType(executionContext);

if (containerType && visualizationType) {
plugins.usageCollection?.reportUiCounter(containerType, METRIC_TYPE.COUNT, [
`render_${visualizationType}_${type}`,
]);
}

handlers.done();
};

const { GaugeComponent } = await import('../components/gauge_component');
render(
<KibanaThemeProvider theme$={theme.theme$}>
<KibanaThemeProvider theme$={core.theme.theme$}>
<div className="gauge-container" data-test-subj="gaugeChart">
<GaugeComponent
{...config}
formatFactory={getFormatService().deserialize}
chartsThemeService={getThemeService()}
chartsThemeService={plugins.charts.theme}
paletteService={getPaletteService()}
renderComplete={() => handlers.done()}
renderComplete={renderComplete}
uiState={handlers.uiState as PersistedState}
/>
</div>
Expand Down
19 changes: 14 additions & 5 deletions src/plugins/chart_expressions/expression_gauge/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
import { ChartsPluginSetup } from '@kbn/charts-plugin/public';
import { ChartsPluginSetup, ChartsPluginStart } from '@kbn/charts-plugin/public';
import { CoreSetup, CoreStart } from '@kbn/core/public';
import { Plugin as ExpressionsPublicPlugin } from '@kbn/expressions-plugin/public';
import type { FieldFormatsStart } from '@kbn/field-formats-plugin/public';
import type { UsageCollectionStart } from '@kbn/usage-collection-plugin/public';
import { createStartServicesGetter } from '@kbn/kibana-utils-plugin/public';
import { gaugeFunction } from '../common';
import { setFormatService, setThemeService, setPaletteService } from './services';
import { setFormatService, setPaletteService } from './services';
import { gaugeRenderer } from './expression_renderers';

/** @internal */
Expand All @@ -22,18 +24,25 @@ export interface ExpressionGaugePluginSetup {
/** @internal */
export interface ExpressionGaugePluginStart {
fieldFormats: FieldFormatsStart;
usageCollection?: UsageCollectionStart;
charts: ChartsPluginStart;
}

/** @internal */
export class ExpressionGaugePlugin {
public setup(core: CoreSetup, { expressions, charts }: ExpressionGaugePluginSetup) {
setThemeService(charts.theme);
public setup(
core: CoreSetup<ExpressionGaugePluginStart, void>,
{ expressions, charts }: ExpressionGaugePluginSetup
) {
charts.palettes.getPalettes().then((palettes) => {
setPaletteService(palettes);
});
const getStartDeps = createStartServicesGetter<ExpressionGaugePluginStart, void>(
core.getStartServices
);

expressions.registerFunction(gaugeFunction);
expressions.registerRenderer(gaugeRenderer({ theme: core.theme }));
expressions.registerRenderer(gaugeRenderer({ getStartDeps }));
}

public start(core: CoreStart, { fieldFormats }: ExpressionGaugePluginStart) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,4 @@
*/

export { getFormatService, setFormatService } from './format_service';
export { getThemeService, setThemeService } from './theme_service';
export { getPaletteService, setPaletteService } from './palette_service';
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@
"server/**/*",
],
"references": [
{ "path": "../tsconfig.json" },
{ "path": "../../../core/tsconfig.json" },
{ "path": "../../expressions/tsconfig.json" },
{ "path": "../../usage_collection/tsconfig.json" },
{ "path": "../../presentation_util/tsconfig.json" },
{ "path": "../../field_formats/tsconfig.json" },
{ "path": "../../charts/tsconfig.json" },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type { HeatmapArguments } from '..';
import { functionWrapper } from '@kbn/expressions-plugin/common/expression_functions/specs/tests/utils';
import { Datatable } from '@kbn/expressions-plugin/common/expression_types/specs';
import { EXPRESSION_HEATMAP_GRID_NAME, EXPRESSION_HEATMAP_LEGEND_NAME } from '../constants';
import { ExecutionContext } from '@kbn/expressions-plugin';

describe('interpreter/functions#heatmap', () => {
const fn = functionWrapper(heatmapFunction());
Expand Down Expand Up @@ -56,7 +57,7 @@ describe('interpreter/functions#heatmap', () => {
};

it('returns an object with the correct structure', () => {
const actual = fn(context, args, undefined);
const actual = fn(context, args);

expect(actual).toMatchSnapshot();
});
Expand All @@ -72,8 +73,10 @@ describe('interpreter/functions#heatmap', () => {
reset: () => {},
},
},
};
await fn(context, args, handlers as any);
getExecutionContext: jest.fn(),
} as unknown as ExecutionContext;

await fn(context, args, handlers);

expect(loggedTable!).toMatchSnapshot();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@
"ui": true,
"requiredPlugins": ["expressions", "fieldFormats", "charts", "visualizations", "presentationUtil", "data"],
"requiredBundles": ["kibanaUtils", "kibanaReact"],
"optionalPlugins": [],
"optionalPlugins": ["usageCollection"],
"extraPublicDirs": ["common"]
}
Loading