-
Notifications
You must be signed in to change notification settings - Fork 937
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
base: main
Are you sure you want to change the base?
Changes from 11 commits
8913f15
bb50042
49713ef
fb42ef5
3c0799e
3d5dddc
063d9a1
20dbacf
6636bf9
b937c42
3f15a5f
4aa842b
33b9660
7f59b70
4b63789
20957e7
1bcb100
664d4de
f6c03a7
846bccf
5f785ae
c566adf
1f70a7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
name: Performance Testing | ||
|
||
on: | ||
pull_request: | ||
branches: | ||
- main | ||
|
||
env: | ||
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 &" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
- 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 }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
const baseUrl = 'http://localhost:5601'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need a license here? (like .eslintrc.js) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
}, | ||
}, | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which OS instance does this connect to? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 . |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: no changes observed besides white spacing |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
@@ -464,6 +465,7 @@ | |
"postcss": "^8.4.5", | ||
"prettier": "^2.1.1", | ||
"prop-types": "^15.7.2", | ||
"puppeteer": "^24.1.1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need: