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

[Security Solution] Add custom features to lens #138995

Merged
merged 36 commits into from
Nov 2, 2022

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Aug 17, 2022

Summary

Security Solution is migrating existing charts to Lens Embeddables but keep the custom inspectors.
In this PR we made some changes to allow Embeddables to use a custom inspector, and allow metric to align left or center:

To see the result of Lens Embeddables on Security Solutions, please run #138995, add this to kibana.dev.yml, and visit any of the explore pages:

xpack.securitySolution.enableExperimental: [chartEmbeddablesEnabled]
Screen.Recording.2022-10-27.at.15.11.41.mov

Screenshot 2022-10-28 at 15 00 58

@angorayc angorayc added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.5.0 labels Aug 17, 2022
@angorayc angorayc added v8.6.0 and removed v8.5.0 labels Oct 28, 2022
@angorayc angorayc marked this pull request as ready for review October 28, 2022 14:05
@angorayc angorayc requested a review from a team as a code owner October 28, 2022 14:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Hi @angorayc ! First off, thank you for taking the initiative to contribute "upstream" to Lens to unblock yourself from using the embeddable!

You're on the right track, but we should change how the metric alignment parameter makes it to the metric-component.tsx.

Currently you have it set up like this:

Since the metric alignment parameter only applies to the legacy metric vis type, we should change it so that metricAlignment becomes part of the lensAttributes instead of being part of the embeddable input. So, we should

Add metricAlignment as an optional argument to the legacy metric expression function

first it needs to be added to the type

export interface MetricArguments {
percentageMode: boolean;
colorMode: ColorMode;
showLabels: boolean;
palette?: PaletteOutput<CustomPaletteState>;
font: Style;
labelFont: Style;
labelPosition: LabelPositionType;
metric: Array<ExpressionValueVisDimension | string>;
bucket?: ExpressionValueVisDimension | string;
colorFullBackground: boolean;
autoScale?: boolean;
}

then, to the expression function args

Get the value of the alignment from the args instead of the handlers.variables

Add alignment as an optional member in the legacy metric state

export interface LegacyMetricState {
layerId: string;
accessor?: string;
layerType: LayerType;
colorMode?: ColorMode;
palette?: PaletteOutput<CustomPaletteParams>;
titlePosition?: 'top' | 'bottom';
size?: string;
textAlign?: 'left' | 'right' | 'center';
}

Attach the metricAlignment argument to the expression AST when present

Let me know if you have any questions about how this all fits together!

@angorayc angorayc requested a review from a team as a code owner October 31, 2022 12:11
@angorayc angorayc changed the title Lens embeddables/enhancement [Security Solution] Add custom styles to lens Oct 31, 2022
@angorayc angorayc changed the title [Security Solution] Add custom styles to lens [Security Solution] Add custom features to lens Oct 31, 2022
@angorayc angorayc added Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore labels Oct 31, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Looking good! Left a couple of comments.

@flash1293 do you know who owns the interpreter_functional tests? @angorayc ran the functional tests with the updateBaselines option locally and it seems like it has changed a bunch of visual baselines that weren't failing before.

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

LGTM!

@angorayc angorayc enabled auto-merge (squash) November 2, 2022 10:37
@kibana-ci
Copy link
Collaborator

💚 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
expressionLegacyMetricVis 49 51 +2
lens 583 585 +2
total +4

Async chunks

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

id before after diff
expressionLegacyMetricVis 11.6KB 12.0KB +385.0B
lens 1.3MB 1.3MB +113.0B
securitySolution 9.6MB 9.6MB +128.0B
total +626.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
expressionLegacyMetricVis 1 2 +1

Page load bundle

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

id before after diff
expressionLegacyMetricVis 9.8KB 10.1KB +282.0B
lens 29.2KB 29.3KB +58.0B
total +340.0B
Unknown metric groups

API count

id before after diff
expressionLegacyMetricVis 49 51 +2
lens 676 678 +2
total +4

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 57 63 +6
osquery 103 108 +5
securitySolution 439 443 +4
total +17

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 65 71 +6
osquery 104 110 +6
securitySolution 516 520 +4
total +18

History

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

@angorayc angorayc merged commit e6ae2f2 into elastic:main Nov 2, 2022
@flash1293
Copy link
Contributor

do you know who owns the interpreter_functional tests

Sorry for being late to the party - I would say appservices which means they are now back to the viseditors/visualizations team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants