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

Lighthouse performance metrics setup #9304

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

ruchidh
Copy link

@ruchidh ruchidh commented Jan 30, 2025

Description

This CR is testing github workflow for performance testing, currently for job lighthouse.
This is not for merge purpose as of now. Please ignore license checks or other issues for now.

Issues Resolved

Screenshot

Testing the changes

Changelog

  • skip

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

target: 'temporary-public-storage', // Required for GitHub integration
},
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

which OS instance does this connect to?

Copy link
Author

@ruchidh ruchidh Jan 31, 2025

Choose a reason for hiding this comment

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

for our BE, 9200. This temporary-public-storage , is a public instance of google report .

module.exports = {
ci: {
collect: {
url: [`${baseUrl}`, `${baseUrl}/app/data-explorer/discover`, `${baseUrl}/app/dashboards`], // Add more URLs as needed
Copy link
Contributor

@angle943 angle943 Jan 30, 2025

Choose a reason for hiding this comment

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

Something i'm wondering about...

Without some specific actions, I don't think any results would be loaded in discover and no dashboards will be set up on dashboards page, so i'm not sure if we are getting the full benefit of having this.

Is it possible to start with some examples that'll load? perhaps the sample data (but even with that i think we need to modify the URL so we are hitting the correct page. For example /app/dashboards might just go to the list of dashboards you need to select from)

Screenshot 2025-01-30 at 3 30 51 PM

Copy link
Member

Choose a reason for hiding this comment

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

+1, you can actually hit the api or bulk insert saved objects and data

Copy link
Author

Choose a reason for hiding this comment

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

yes, have plan to do so, updating mock data

assert: {
preset: 'lighthouse:recommended',
assertions: {
performance: ['error', { minScore: 0.9 }],
Copy link
Contributor

Choose a reason for hiding this comment

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

oof i think we need make the minscore much lower. and it might differ between the different URLs.

For example on dashboards page with the example data loaded, i am getting a score of 34 (yikes)

Copy link
Author

Choose a reason for hiding this comment

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

This is only for testing purpose, would change in final review.

@@ -0,0 +1,51 @@
name: Performance Testing
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need:

# Copyright OpenSearch Contributors
# SPDX-License-Identifier: Apache-2.0

@@ -0,0 +1,28 @@
const baseUrl = 'http://localhost:5601';
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a license here? (like .eslintrc.js)

@@ -464,6 +465,7 @@
"postcss": "^8.4.5",
"prettier": "^2.1.1",
"prop-types": "^15.7.2",
"puppeteer": "^24.1.1",
Copy link
Member

Choose a reason for hiding this comment

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

i would be hesitant to add this. i know some opensearch plugin uses puppeteer and they have to bundle a chrome binary so that it can not fail on some integration testing.

Copy link
Member

Choose a reason for hiding this comment

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

i think other plugin teams have used puppeteer-core? not sure haven't looked too deep into it

Copy link
Author

Choose a reason for hiding this comment

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

okay, let's discuss this

Signed-off-by: Ruchi Sharma <[email protected]>

- name: Run OpenSearch
run: |
/bin/bash -c "./opensearch-2.17.0/opensearch-tar-install.sh &"
Copy link
Member

Choose a reason for hiding this comment

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

we should fix this if we have this somewhere else but if a different version then than 2.17 then this will not be right

run: yarn global add @lhci/cli

- name: Build OpenSearch Dashboards
run: yarn build
Copy link
Member

Choose a reason for hiding this comment

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

build will build a platform distribution. are we sure we dont want to yarn osd bootstrap?

sleep 30
shell: bash

- name: Install Lighthouse CI
Copy link
Member

Choose a reason for hiding this comment

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

is there not a github action that we can pull from the marketplace and run this?

@@ -0,0 +1,28 @@
const baseUrl = 'http://localhost:5601';
Copy link
Member

Choose a reason for hiding this comment

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

baseUrl technically doesn't have to be 5601. It can be configurable to be something like 443

# data.savedQueriesNewUI.enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

nit: no changes observed besides white spacing

@@ -271,6 +271,7 @@
"@elastic/filesaver": "1.1.2",
"@elastic/makelogs": "^6.1.0",
"@faker-js/faker": "^7.6.0",
"@lhci/cli": "^0.14.0",
Copy link
Member

Choose a reason for hiding this comment

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

if this is a dev dependency then bootstrapping should be suffice enough correct? do we still need to add it the step to install it globally in the github workflow?

@angle943
Copy link
Contributor

We talked about this in private, but just for some permanence, I will write TLDR version here. Some challenges with this approach are:

  • installing mock data to load the data
  • Telling Lighthouse the correct URL to see the loaded data (challenge is that for dashboard, the created dashboards have randomly generated IDs in the URL. For Discover, we will need to modify the URL to have all the correct data and timestamps to load the proper data)

One way we can solve this is leveraging Cypress: https://medium.com/@mudit56p/enhancing-ui-performance-testing-with-lighthouse-and-cypress-1ad69db0a150

Signed-off-by: Ruchi Sharma <[email protected]>
Signed-off-by: Ruchi Sharma <[email protected]>
Signed-off-by: Ruchi Sharma <[email protected]>
Signed-off-by: Ruchi Sharma <[email protected]>
Signed-off-by: Ruchi Sharma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants