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.

Sign up for GitHub

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

Jump to bottom

[Exploratory view] use percentages in distribution chart #103080

Merged
merged 23 commits into from
Jun 29, 2021
Merged

[Exploratory view] use percentages in distribution chart #103080

merged 23 commits into from
Jun 29, 2021

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jun 23, 2021 •
edited
Loading

Summary

fix #103469

should be merged/reviwed after #103214

Using lens formula overall count implemented to display percentages in Distribution charts on Y axis in exploratory view.

image

@shahzad31 shahzad31 changed the title use percentages overall [Exploratory view] use percentages in distribution chart Jun 28, 2021
@shahzad31 shahzad31 self-assigned this Jun 28, 2021
@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 28, 2021
@shahzad31 shahzad31 marked this pull request as ready for review June 28, 2021 12:21
@shahzad31 shahzad31 requested a review from a team June 28, 2021 12:21
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31 shahzad31 requested a review from justinkambic June 28, 2021 12:22
flash1293
flash1293 approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Kibana app changes LGTM

dominiqueclarke
dominiqueclarke reviewed Jun 28, 2021
View reviewed changes
...ns/observability/public/components/shared/exploratory_view/configurations/lens_attributes.ts
@@ -396,10 +399,14 @@ export class LensAttributes {
}
}

getMainYAxis(layerConfig: LayerConfig) {
getMainYAxis(layerConfig: LayerConfig, layerId: string, columnFilter: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I fully understand the idea of a mainYAxis and a childYAxis of yet. Can you explain to me how they relate to one another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry again for confusing naming here. Seems like i need to clean up this part as well.

So as far as Lens is concerned there are no main/child axis. I coined these because i thought we will have a main series concept in lens.

Essentially child columns here means there are more than one column in the layer.

...ns/observability/public/components/shared/exploratory_view/configurations/lens_attributes.ts Outdated
@@ -594,7 +608,7 @@ export class LensAttributes {
layers: this.layerConfigs.map((layerConfig, index) => ({
accessors: [
`y-axis-column-layer${index}`,
...Object.keys(this.getChildYAxises(layerConfig)),
// ...Object.keys(this.getChildYAxises(layerConfig)),
Copy link
Contributor

@dominiqueclarke dominiqueclarke Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Did you intend to comment this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, i removed it now.

...ity/public/components/shared/exploratory_view/configurations/lens_columns/overall_column.tsx
string,
CountIndexPatternColumn | MathIndexPatternColumn | OverallSumIndexPatternColumn
> = {
[`${yAxisColId}X0`]: countColumn,
Copy link
Contributor

Choose a reason for hiding this comment

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

the name column is still confusing to me, but I know it maps back to lens nicely.

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 will probably spend more time in tech sync or dedicated session to go over this concept.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 246 247 +1

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
lens 162 169 +7

Async chunks

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

id before after diff
observability 472.2KB 474.4KB +2.2KB

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
lens 16 14 -2
Unknown metric groups

API count

id before after diff
lens 174 181 +7

History

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

cc @shahzad31

@shahzad31 shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 28, 2021
justinkambic
justinkambic approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

Smoke test went well; I'm ++ to @dominique in thinking we should continue sessions on this codebase as a lot of it seems arcane to me.

@shahzad31 shahzad31 merged commit 409a0f2 into elastic:master Jun 29, 2021
@shahzad31 shahzad31 deleted the percentages-over-all branch June 29, 2021 09:21
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 29, 2021
@shahzad31 @kibanamachine
[Exploratory view] use percentages in distribution chart (elastic#103080
f63841d 
)
@kibanamachine kibanamachine mentioned this pull request Jun 29, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 29, 2021
@kibanamachine @shahzad31
[Exploratory view] use percentages in distribution chart (#103080) (#…
1339f85 
…103625)

Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinkambic justinkambic justinkambic approved these changes

@dominiqueclarke dominiqueclarke dominiqueclarke approved these changes

@flash1293 flash1293 flash1293 approved these changes

Assignees

@shahzad31 shahzad31

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Exploratory view] Displays distribution as percentages in charts
6 participants
@shahzad31 @elasticmachine @kibanamachine @flash1293 @dominiqueclarke @justinkambic