-
Notifications
You must be signed in to change notification settings - Fork 535
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
refactor(ci): Move telemetry-generator out of the public repo #23411
refactor(ci): Move telemetry-generator out of the public repo #23411
Conversation
…h(s) to telemetry-generator.
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.
Copilot reviewed 30 out of 45 changed files in this pull request and generated 4 comments.
Files not reviewed (15)
- biome.jsonc: Language not supported
- tools/telemetry-generator/LICENSE: Language not supported
- tools/telemetry-generator/package.json: Language not supported
- tools/telemetry-generator/.eslintrc.cjs: Evaluated as low risk
- tools/telemetry-generator/prettier.config.cjs: Evaluated as low risk
- tools/telemetry-generator/README.md: Evaluated as low risk
- tools/telemetry-generator/.mocharc.cjs: Evaluated as low risk
- tools/pipelines/build-client.yml: Evaluated as low risk
- tools/pipelines/templates/build-npm-package.yml: Evaluated as low risk
- tools/pipelines/templates/include-telemetry-setup.yml: Evaluated as low risk
- tools/pipelines/templates/include-test-perf-benchmarks.yml: Evaluated as low risk
- tools/pipelines/templates/include-upload-stage-telemetry.yml: Evaluated as low risk
- tools/pipelines/templates/include-vars-telemetry-generator.yml: Evaluated as low risk
- tools/pipelines/templates/include-vars.yml: Evaluated as low risk
- fluidBuild.config.cjs: Evaluated as low risk
# Absolute path to the folder that contains the source code for the telemetry-generator package, which is | ||
# used in a few places in the pipeline to push custom telemetry to Kusto. | ||
- name: absolutePathToTelemetryGenerator | ||
value: $(Build.SourcesDirectory)/tools/telemetry-generator | ||
readonly: true |
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.
Moved (and renamed) the variable to the new include-vars-telemetry-generator.yml
template, which is in turn included by include-vars.yml
. Similar snippets were removed in other yml files for the same reason.
@@ -522,26 +517,27 @@ extends: | |||
script: | | |||
set -eu -o pipefail | |||
echo "Build Directory is ${{ parameters.buildDirectory }}"; | |||
BUNDLE_SIZE_TESTS_DIR="${{ parameters.buildDirectory }}/artifacts/bundleAnalysis/@fluid-example/bundle-size-tests"; | |||
BUNDLE_SIZE_TESTS_DIR="$(Build.ArtifactStagingDirectory)/bundleAnalysis/@fluid-example/bundle-size-tests"; |
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.
The task in line 488 already copies the files to this location; giving it preference over the original source location since it's easier to reference it as an absolute path, which makes it easier to use below in line 540 when passing it to telemetry-generator.
# If the template should try to checkout the repo. If the job that includes this template already has steps to check out | ||
# the repo for other purposes, this should be set to false. | ||
- name: isCheckoutNeeded | ||
type: boolean | ||
default: false |
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.
Was relevant for pipelines that did not check out the repo for their functionality (e.g. running e2e tests) but needed to do it in order to get access to telemetry-generator. Now they install it from an internal ADO feed so they don't need to check out the repo any more.
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.
Made this file its own new template instead of putting the variables directly in include-vars.yml
because test-pipelines also need these, and they don't currently include include-vars.yml
(probably that was the intention from the start; the variables there are related to building and publishing).
ls -la '../../examples/utils/bundle-size-tests/bundleAnalysis/report.json'; | ||
node --require @ff-internal/aria-logger bin/run --handlerModule $(absolutePathToTelemetryGenerator)/dist/handlers/bundleSizeHandler.js --dir '../../artifacts/bundleAnalysis/@fluid-example/bundle-size-tests'; | ||
ls -la '$(Build.SourcesDirectory)/examples/utils/bundle-size-tests/bundleAnalysis/report.json'; | ||
npx telemetry-generator --handlerModule "$(pathToTelemetryGeneratorHandlers)/bundleSizeHandler.js" --dir '$(Build.ArtifactStagingDirectory)/bundleAnalysis/@fluid-example/bundle-size-tests'; |
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.
The telemetry-generator package published to our internal feeds now also exposes a telemetry-generator
executable that can be called from npm/npx scripts.
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
Description
Moves the telemetry-generator tool out of the public repo. It now lives in an internal repository and pipelines now install it from our internal ADO feeds.
Reviewer Guidance
The review process is outlined on this wiki page.
Confirmed with a test/ branch with these changes (plus a couple adjustments to the build-client pipeline so I could test appropriately) that all the expected telemetry (pipeline stage information, perf benchmark results, bundle sizes) shows up in Kusto as expected.
AB#4243
AB#26776