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

[APM] Replace Lens embeddable charts used for mobile services #152139

Closed
gbamparop opened this issue Feb 24, 2023 · 13 comments · Fixed by #155026
Closed

[APM] Replace Lens embeddable charts used for mobile services #152139

gbamparop opened this issue Feb 24, 2023 · 13 comments · Fixed by #155026
Assignees
Labels
8.8 candidate apm:mobile apm:release-feature APM UI - Release Feature Goal apm:stretch-goal apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 apm:test-plan-done Pull request that was successfully tested during the test plan Team:APM All issues that need APM UI Team support

Comments

@gbamparop
Copy link
Contributor

gbamparop commented Feb 24, 2023

In 8.6 we have introduced the following charts for mobile services using Lens embeddables:

  • most used devices
  • most used network connection type
  • most used OS version
  • most used app version

image

A stretch goal for 8.8 will be to replace the lens embeddables with elastic-charts.

Links

@gbamparop gbamparop added Team:APM All issues that need APM UI Team support apm:mobile apm:release-feature APM UI - Release Feature Goal 8.8 candidate apm:stretch-goal labels Feb 24, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@achyutjhunjhunwala achyutjhunjhunwala self-assigned this Apr 4, 2023
@achyutjhunjhunwala
Copy link
Contributor

achyutjhunjhunwala commented Apr 4, 2023

@gbamparop @kpatticha Can you confirm the following behaviour.
When we replace the current Lens Embeddable implementation with elastic-charts -

  1. We don't want these charts top open in Lens any more and hence we will get rid of the context menu as well

image

  1. Is the Sunburst chart the recommended chart to use here - https://elastic.github.io/elastic-charts/?path=/story/sunburst--ordered-slices&globals=theme:light
  2. We will have to add our own API implementation (ES Query) to retrieve this data as previously i can see we were passing a prepared statement to Lens Embeddable to make the query. Also pointed here
    [Update] I will use the existing get_mobile_filters endpoint to retrieve this information. Will simply modify it for my use case

@gbamparop
Copy link
Contributor Author

gbamparop commented Apr 4, 2023

  1. We don't want these charts top open in Lens any more and hence we will get rid of the context menu as well

This is only available with lens embeddable so we won't have it with elastic charts.

  1. Is the Sunburst chart the recommended chart to use here - https://elastic.github.io/elastic-charts/?path=/story/sunburst--ordered-slices&globals=theme:light

The ones above seem like donut charts. @boriskirov would you rather have something else instead now that we're moving away from the embeddables? We've discussed custom charts before but I think it's good to pick one which is available in elastic-charts.

@kpatticha
Copy link
Contributor

I added the Figma link in the ticket description as a reference.

In the mockups, the following components were suggested.
image

UX solution uses similar components and we might able to reuse the component. it seems it is defined.

I suggest, if we can reuse the component to go in that direction otherwise we can explore more straightforward alternative.

@sorenlouv
Copy link
Member

@gbamparop Can you add the reasoning for replacing lens embeddables with elastic charts? In general we want to move towards embeddables because users requests it (enables them to modify the visualisations) so there should be good reasons for moving away from them.

@kpatticha
Copy link
Contributor

@sqren I tried to summarize the pros/cons here

Also, there was an internal discussion about Lens with the observability folks (Observability UI Weekly 23/02)

@gbamparop
Copy link
Contributor Author

@gbamparop Can you add the reasoning for replacing lens embeddables with elastic charts? In general we want to move towards embeddables because users requests it (enables them to modify the visualisations) so there should be good reasons for moving away from them.

That was our intention when we started introducing charts for mobile, as @kpatticha mentions above after some analysis we have discussed with the team and decided to use custom charts instead. I think one of the main reasons is the support for comparisons so although introducing this functionality is not part of this issue, it'll be great if we pick a chart type where the design team has a path forwards about where the comparisons should be displayed.

@achyutjhunjhunwala
Copy link
Contributor

@boriskirov Can you help us here picking the right chart based on the provided comments above ?

@achyutjhunjhunwala
Copy link
Contributor

@isaclfreire I just realised that Boris is on PTO, could you help us here ?

@achyutjhunjhunwala
Copy link
Contributor

achyutjhunjhunwala commented Apr 11, 2023

Currently the Most Used Devices section only display the top 5 items and the percentage is summed up for those 5 devices to a 100%. As discussed with @akhileshpok, we will also display other options to cover the results returned by the sum_other_doc_count count as well

Also @akhileshpok confirmed we will not be displaying comparisons on this chart. Hence i will add the tooltip, comparisons not supported

@isaclfreire In that case the only feedback i need from you is to choose the right chart for this implementation, i believe this is the one which we will be using - https://elastic.github.io/elastic-charts/?path=/story/sunburst--with-direct-text-labels&globals=theme:light

cc: @gbamparop @kpatticha

achyutjhunjhunwala added a commit that referenced this issue Apr 20, 2023
## Summary

This PR closes #152139

This change brings a big performance improvement in Loading of the
Charts

### Checklist

- [x] Add new endpoint to retrieve filtered data based on URL params
- [x] Replace Embeddables with Elastic Charts
- [x] Delete existing code for Embeddables
- [x] Handle Loaders
- [x] Add similar No results found visualisations
- [x] Add Cy Tests
- [x] Add API Tests

## Demo



https://user-images.githubusercontent.com/7416358/232797685-1b009d5d-cd4a-4041-aa33-872647491ced.mov

---------

Co-authored-by: kibanamachine <[email protected]>
@gbamparop gbamparop added the apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 label Apr 25, 2023
@achyutjhunjhunwala
Copy link
Contributor

achyutjhunjhunwala commented May 3, 2023

The charts loads properly though i have my concerns around the data.

@sqren @dgieselaar I compared the queries fired by us and by lens team to get Most Used Charts for Device Identifier (just one example). I see a difference in response for the chart values

image

Query we fired is
image

Query fired by Lens is
image

The difference i see is we filter the terms by "processor.event": ["transaction"] and must query for {"term": {"transaction.type": "mobile"} ,

Due to that the difference in response is present. Is my assumption correct that our new queries are correct compared to what we were passing to Lens previously and that i can mark this as done ?

@achyutjhunjhunwala achyutjhunjhunwala self-assigned this May 3, 2023
@sorenlouv
Copy link
Member

@achyutjhunjhunwala Thanks for the thorough testing! Do you get the same numbers if you remove the filters "processor.event": ["transaction"] and {"term": {"transaction.type": "mobile"}?

It sounds right that those filters should be applied so it was probably a (small) bug that they weren't in the Lens implementation.

@achyutjhunjhunwala
Copy link
Contributor

Not exact, but closer. I will mark this ticket test done in that case

@achyutjhunjhunwala achyutjhunjhunwala added the apm:test-plan-done Pull request that was successfully tested during the test plan label May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.8 candidate apm:mobile apm:release-feature APM UI - Release Feature Goal apm:stretch-goal apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 apm:test-plan-done Pull request that was successfully tested during the test plan Team:APM All issues that need APM UI Team support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants