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

Fix preview chart in APM Latency threshold rule #158439

Merged

Conversation

benakansara
Copy link
Contributor

@benakansara benakansara commented May 25, 2023

Resolves the following for APM Latency threshold rule:

Improvements in the preview chart:

  • Showing data grouped by selected group by fields
  • Showing 5 groups from each bucket
  • Showing last x units (minutes, hours, etc.) data
  • Added loading indicator
  • Showing tooltip
  • Always showing legend
  • Max Y is slightly higher than maximum of highest y value or threshold
  • Adjusting chart to always show the threshold line
  • Fixed
    • When removing some fields like transaction type, transaction name, chart showed no data
    • Hiding a particular group (by clicking on legend item), readjusted max Y value, but same was not reset to original when enabling the group back

Before

Screen.Recording.2023-05-30.at.09.59.48.mov

After

Screen.Recording.2023-05-30.at.10.14.19.mov

@benakansara benakansara requested a review from a team May 25, 2023 09:03
@benakansara benakansara self-assigned this May 25, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@benakansara benakansara added release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.9.0 labels May 25, 2023
@benakansara benakansara marked this pull request as ready for review May 30, 2023 08:20
@benakansara benakansara requested a review from a team as a code owner May 30, 2023 08:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/actionable-observability (Team: Actionable Observability)

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label May 30, 2023
@elasticmachine
Copy link
Contributor

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

@sorenlouv
Copy link
Member

/oblt-deploy

@sorenlouv
Copy link
Member

sorenlouv commented May 31, 2023

Showing 3 groups from each bucket

From the video it looks like we are showing 10 groups (there are 10 legends). Am I missing something?

EDIT: Ahh, is it because the groups are not constant between the buckets? Would be great if for the preview we could select 3 groups, and show only those 3 for every bucket.

image

@benakansara
Copy link
Contributor Author

From the video it looks like we are showing 10 groups (there are 10 legends). Am I missing something?

EDIT: Ahh, is it because the groups are not constant between the buckets? Would be great if for the preview we could select 3 groups, and show only those 3 for every bucket.

yes, the groups are not constant. I will check if we can keep the selected groups constant.

@benakansara
Copy link
Contributor Author

/oblt-deploy

@fkanout fkanout self-requested a review June 5, 2023 08:32
Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

Tested locally, and it works as expected. However, I left some comments

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that this file/getIntervalInSeconds function is used in other rules too but with no unit test, I assume it's just working 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests 71f62fd

@sorenlouv
Copy link
Member

sorenlouv commented Jun 13, 2023

My bad; two PRs are fixing the APM preview chart!

Now worries. And perhaps this is a good indication that the empty state can be improved :)

@benakansara
Copy link
Contributor Author

@sqren We hid the preview charts in APM rules in 8.8.0. I think @fkanout was checking the PR where chart was completely hidden, not in empty state.
For empty state, the chart will show "No data to display".

@fkanout fkanout self-requested a review June 13, 2023 14:47
Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

I tested it, and it works as expected, in the Alert details page and in the rule flyout. Great job, @benakansara!
Screenshot 2023-06-13 at 16 57 47

Screenshot 2023-06-13 at 16 52 22

@sorenlouv
Copy link
Member

I think @fkanout was checking the PR where chart was completely hidden, not in empty state. For empty state, the chart will show "No data to display".

Ahh, perfect!!

@benakansara benakansara requested a review from sorenlouv June 14, 2023 10:00
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

all comments made. lgtm 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
apm 3.6MB 3.6MB +420.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
apm 113 112 -1
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 411 415 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 494 498 +4
total +6

History

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

cc @benakansara

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: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Team:APM All issues that need APM UI Team support v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants