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
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions .github/workflows/performance_testing.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
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


on:
pull_request:
branches:
- main

env:
NODE_OPTIONS: '--max-old-space-size=6144 --dns-result-order=ipv4first'
LATEST_VERSION: '2.17.0'

jobs:
lighthouse:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v2

- name: Setup Node
uses: actions/setup-node@v2
with:
node-version-file: '.nvmrc'
registry-url: 'https://registry.npmjs.org'

- name: Setup Yarn
run: |
npm uninstall -g yarn
npm i -g [email protected]

- name: Run bootstrap
run: yarn osd bootstrap

- name: Download OpenSearch
uses: suisei-cn/[email protected]
with:
url: https://artifacts.opensearch.org/releases/bundle/opensearch/${{ env.LATEST_VERSION }}/opensearch-${{ env.LATEST_VERSION }}-linux-x64.tar.gz

- name: Extract OpenSearch
run: |
tar -xzf opensearch-*.tar.gz
rm -f opensearch-*.tar.gz
shell: bash

- name: Remove security plugin
run: |
/bin/bash -c "yes | ./opensearch-${{ env.LATEST_VERSION }}/bin/opensearch-plugin remove opensearch-security"
shell: bash

- 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

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?

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?


- name: Start OpenSearch Dashboards
run: |
yarn wait-on http://localhost:9200
yarn start --no-base-path & yarn wait-on http://localhost:5601

- name: Run Lighthouse CI
run: yarn lhci autorun || echo "Lighthouse assertion failed, check results"

- name: Post Lighthouse Report to PR
if: always()
run: |
echo "🚦 **Lighthouse CI Report**" > comment.txt
echo "" >> comment.txt
lhci upload --target=temporary-public-storage | tee lhci_output.txt
REPORT_URL=$(grep -o 'https://storage.googleapis.com/lighthouse-infrastructure-.*' lhci_output.txt | head -n 1)
echo "🔗 [View Full Lighthouse Report]($REPORT_URL)" >> comment.txt
gh pr comment ${{ github.event.pull_request.number }} --body "$(cat comment.txt)"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
28 changes: 28 additions & 0 deletions .lighthouserc.js
Original file line number Diff line number Diff line change
@@ -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)

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


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

startServerCommand: 'yarn start --no-base-path',
numberOfRuns: 3,
settings: {
chromePath: require('puppeteer').executablePath(),
chromeFlags: '--no-sandbox --disable-gpu --headless',
},
},
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.

'first-contentful-paint': ['warn', { maxNumericValue: 1800 }],
'largest-contentful-paint': ['warn', { maxNumericValue: 2500 }],
interactive: ['warn', { maxNumericValue: 5000 }],
'total-blocking-time': ['warn', { maxNumericValue: 300 }],
},
},
upload: {
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 .

4 changes: 2 additions & 2 deletions config/opensearch_dashboards.yml
Original file line number Diff line number Diff line change
Expand Up @@ -372,10 +372,10 @@

# Set the backend roles in groups or users, whoever has the backend roles or exactly match the user ids defined in this config will be regard as dashboard admin.
# Dashboard admin will have the access to all the workspaces(workspace.enabled: true) and objects inside OpenSearch Dashboards.
# The default config is [], and no one will be dashboard admin.
# The default config is [], and no one will be dashboard admin.
# If the user config is set to wildcard ["*"], anyone will be dashboard admin.
# opensearchDashboards.dashboardAdmin.groups: ["dashboard_admin"]
# opensearchDashboards.dashboardAdmin.users: ["dashboard_admin"]

# Set the value to true to enable the new UI for savedQueries in Discover
# data.savedQueriesNewUI.enabled: true
# data.savedQueriesNewUI.enabled: true
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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?

"@microsoft/api-documenter": "^7.13.78",
"@microsoft/api-extractor": "^7.19.3",
"@osd/babel-preset": "1.0.0",
Expand Down Expand Up @@ -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

"react": "^16.14.0",
"react-grid-layout": "^0.16.2",
"react-markdown": "^4.3.1",
Expand Down
Loading
Loading