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 all 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
4 changes: 2 additions & 2 deletions .github/workflows/cypress_workflow.yml
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ jobs:
'node scripts/opensearch_dashboards --dev --no-base-path --no-watch --savedObjects.maxImportPayloadBytes=10485760 --server.maxPayloadBytes=1759977 --logging.json=false --data.search.aggs.shardDelay.enabled=true' ||
'node ../scripts/opensearch_dashboards --dev --no-base-path --no-watch --savedObjects.maxImportPayloadBytes=10485760 --server.maxPayloadBytes=1759977 --logging.json=false --data.search.aggs.shardDelay.enabled=true --csp.warnLegacyBrowsers=false --uiSettings.overrides["query:enhancements:enabled"]=false' }}
OPENSEARCH_SNAPSHOT_CMD: ${{ matrix.config == 'query_enhanced' &&
'/bin/bash -c "./opensearch-2.17.0/opensearch-tar-install.sh &"' ||
'/bin/bash -c "./opensearch-${{ env.LATEST_VERSION }}/opensearch-tar-install.sh &"' ||
matrix.config == 'dashboard' &&
'node scripts/opensearch snapshot -E cluster.routing.allocation.disk.threshold_enabled=false' ||
'node ../scripts/opensearch snapshot -E cluster.routing.allocation.disk.threshold_enabled=false' }}
Expand Down Expand Up @@ -235,7 +235,7 @@ jobs:
- name: Run OpenSearch
if: matrix.config == 'query_enhanced'
run: |
/bin/bash -c "./opensearch-2.17.0/opensearch-tar-install.sh &"
/bin/bash -c "./opensearch-${{ env.LATEST_VERSION }}/opensearch-tar-install.sh &"
sleep 30
shell: bash

Expand Down
108 changes: 108 additions & 0 deletions .github/workflows/performance_testing.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
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-${{ env.LATEST_VERSION }}/opensearch-tar-install.sh &"
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 add --dev @lhci/cli

- name: Run bootstrap
run: yarn osd bootstrap

- name: Build plugins
run: node scripts/build_opensearch_dashboards_platform_plugins --no-examples --workers 12

# - name: Start OpenSearch Dashboards
# run: |
# yarn start --no-base-path

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

# - name: Wait for OpenSearch to be ready
# run: |
# until curl -s http://localhost:9200 >/dev/null; do
# echo "Waiting for OpenSearch..."
# sleep 10
# done
# echo "OpenSearch is up!"

# - name: Start OpenSearch Dashboards
# run: |
# yarn start --no-base-path &
# until curl -s http://localhost:5601 >/dev/null; do
# echo "Waiting for OpenSearch Dashboards..."
# sleep 10
# done
# echo "OpenSearch Dashboards is up!"
#

- name: Mock data
run: |
curl 'http://localhost:5601/api/sample_data/ecommerce' -X 'POST' -H 'osd-version: 3.0.0' -H 'osd-xsrf: osd-fetch'

- 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
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

3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@
"tslib": "^2.0.0",
"type-detect": "^4.0.8",
"uuid": "3.3.2",
"wait-on": "^8.0.2",
"whatwg-fetch": "^3.0.0",
"yauzl": "^2.10.0"
},
Expand All @@ -271,6 +272,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 +466,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