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

[Perfomance] Add is_initial_load meta #206645

Merged
merged 27 commits into from
Feb 25, 2025
Merged

Conversation

kpatticha
Copy link
Contributor

@kpatticha kpatticha commented Jan 14, 2025

closes https://github.com/elastic/observability-dev/issues/4185

Summary

This PR adds the is_initial_load parameter to the meta field to distinguish whether the onPageReady trigger occurs during the initial load or a page refresh.

Refactoring:

  • Removed the target field. as context.pageName now provides the necessary information
  • Refactor APM instrumentation to simplify it

Fixes:

⚠️ Instrumentation

The plugins need to call the following function:

onPageRefreshStart()

This method adds a performance marker start::pageRefresh to indicate when a page refresh begins. This marker is used along with an end marker end::pageReady to determine the total refresh duration.

Screen.Recording.2025-02-14.at.12.39.54.PM.mov
Screen.Recording.2025-02-14.at.12.43.29.PM.mov

How to test

  • Checkout the PR
  • make sure you run yarn kbn bootstrap
  • go to any page that has onPageReady function instrumented (ex services)

TODO

  • Once approved, update docs

@kpatticha kpatticha requested a review from a team as a code owner January 14, 2025 17:26
@kpatticha kpatticha added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:current-major Backport to all previous minor branches backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) and removed backport:current-major Backport to all previous minor branches labels Jan 14, 2025
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Approving from the Core POV because a Set is better than a string[].

I added a comment because I'd love to understand what we're trying to track here.

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Awesome! 🥇

Do we also have a way to distinguish whether the event was related to a page load via inside Kibana navigation or the initial load of Kibana?

@kpatticha
Copy link
Contributor Author

Awesome! 🥇

Do we also have a way to distinguish whether the event was related to a page load via inside Kibana navigation or the initial load of Kibana?

with the current implementation we don't have a way to distinguish this. There is another EBT metric, kibana_started, which we might be able to correlate with these two events using their timestamps. However, this would require additional analysis work.

Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

is_initial_load changes with date change because selected time range is persisted via URL state. Also, URL could change even against UI actions which do not cause a data reload.

206645-initial-load-meta-url-issue.mov

See if extracting the plugin url or using pathname is better suited here instead of the full url.

@kpatticha
Copy link
Contributor Author

is_initial_load changes with date change because selected time range is persisted via URL state. Also, URL could change even against UI actions which do not cause a data reload.

206645-initial-load-meta-url-issue.mov
See if extracting the plugin url or using pathname is better suited here instead of the full url.

Thanks, I will take a look

@kpatticha
Copy link
Contributor Author

I discovered a bug while addressing the issue Abdul mentioned. The initial marker isn't removed when the page is refreshed, causing it to report an incorrect duration.

@kpatticha kpatticha requested a review from a team as a code owner February 14, 2025 10:24
@botelastic botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Feb 14, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@kpatticha
Copy link
Contributor Author

Since I found the bug, I refactored and updated the code. I'm requesting another review. 🙏

@kpatticha
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@miloszmarcinkowski miloszmarcinkowski left a comment

Choose a reason for hiding this comment

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

LGTM (code review only)

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Checked locally, LGTM! Thanks for the thorough PR description :)

Is my observation correct that only the highlighted tabs in the following screenshot are instrumented with onPageReady? (I didn't see the ttfmp log on other tabs of this page)

image

performance.clearMarks(perfomanceMarkers.endPageReady);
}

if (
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, is there a scenario in which the onPageReady is called, but none of these two conditions are called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean startPageRefresh ?

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean the onPageReady function, which has this logic; suppose that it is called, but none of these two conditions are met. Is it possible?

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 don't think it's possible. At least the first statement will always be true at least once. In the initial load.

Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

When checking locally, I found that for Metrics Explorer, the refresh duration is very low (it seems it's instantaneous) and doesn't account for the network request time.

The initial load duration looks fine.

206645-Refresh-Duration-Very-Low.mov

* The marker set by this function can later be used in performance measurements
* along with an end marker end::pageReady to determine the total refresh duration.
*/
markPerformanceRefreshStart(): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
markPerformanceRefreshStart(): void;
onPageRefreshStart(): void;

To be consistent with onPageReady and to abstract the performance marking bit. Not a strong opinion 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.

Thanks for the suggestion. I will update

@kpatticha
Copy link
Contributor Author

Checked locally, LGTM! Thanks for the thorough PR description :)

Is my observation correct that only the highlighted tabs in the following screenshot are instrumented with onPageReady? (I didn't see the ttfmp log on other tabs of this page)

image

The service map tab should also report TTFMP .

  • Metrics tab uses static dashboards to show charts for some of the agents
  • Infrastructure tab is in beta and we don't invest much time and effort on it
  • the rest tabs use mainly embeddable for their content

@kpatticha
Copy link
Contributor Author

When checking locally, I found that for Metrics Explorer, the refresh duration is very low (it seems it's instantaneous) and doesn't account for the network request time.

The initial load duration looks fine.

206645-Refresh-Duration-Very-Low.mov

Good catch. Fixed it here 294a3e5

@kpatticha kpatticha requested a review from awahab07 February 18, 2025 16:29
Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

The refresh duration on Metrics Explorer is good now. But the date change is still instantaneous.

206645-Refresh-Date-Change-Issue.mov

@kpatticha
Copy link
Contributor Author

@elasticmachine merge upstream

@kpatticha
Copy link
Contributor Author

The refresh duration on Metrics Explorer is good now. But the date change is still instantaneous.

206645-Refresh-Date-Change-Issue.mov

Screen.Recording.2025-02-25.at.1.35.52.PM.mov

@awahab07 should be fixed now by c9be509 . it's been a bit strange how metrics explorer handles the loading state.

@kpatticha kpatticha requested a review from awahab07 February 25, 2025 11:41
@elasticmachine
Copy link
Contributor

💚 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
@kbn/ebt-tools 29 30 +1

Async chunks

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

id before after diff
apm 2.3MB 2.3MB +83.0B
infra 1.1MB 1.1MB +215.0B
total +298.0B

Page load bundle

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

id before after diff
core 474.6KB 474.6KB +38.0B
kbnUiSharedDeps-srcJs 3.6MB 3.6MB +38.0B
total +76.0B
Unknown metric groups

API count

id before after diff
@kbn/ebt-tools 38 39 +1

History

Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for adding the prop.

@kpatticha kpatticha merged commit 9a1d70d into elastic:main Feb 25, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 9.0

https://github.com/elastic/kibana/actions/runs/13528675783

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
9.0 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 206645

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v9.0.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants