-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Speed up telemetry check by sharing a single TS program across roots !! #267651
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -10,39 +10,65 @@ | |
| import ts from 'typescript'; | ||
| import * as path from 'path'; | ||
| import type { TaskContext } from './task_context'; | ||
| import { extractCollectors, getProgramPaths } from '../extract_collectors'; | ||
| import { | ||
| extractCollectorsWithProgram, | ||
| filterCollectorPaths, | ||
| getProgramPaths, | ||
| } from '../extract_collectors'; | ||
| import { createKibanaProgram } from '../ts_program'; | ||
|
|
||
| export function extractCollectorsTask( | ||
| { roots }: TaskContext, | ||
| restrictProgramToPath?: string | string[] | ||
| ) { | ||
| return roots.map((root) => ({ | ||
| task: async () => { | ||
| const tsConfig = ts.findConfigFile('./', ts.sys.fileExists, 'tsconfig.json'); | ||
| if (!tsConfig) { | ||
| throw new Error('Could not find a valid tsconfig.json.'); | ||
| } | ||
| const programPaths = await getProgramPaths(root.config); | ||
|
|
||
| if (typeof restrictProgramToPath !== 'undefined') { | ||
| const restrictProgramToPaths = Array.isArray(restrictProgramToPath) | ||
| ? restrictProgramToPath | ||
| : [restrictProgramToPath]; | ||
|
|
||
| const fullRestrictedPaths = restrictProgramToPaths.map((collectorPath) => | ||
| path.resolve(process.cwd(), collectorPath) | ||
| ); | ||
| const restrictedProgramPaths = programPaths.filter((programPath) => | ||
| fullRestrictedPaths.includes(programPath) | ||
| return [ | ||
| { | ||
|
Member
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 awesome! I wonder if we can make it even faster by defining concurrent tasks:
Contributor
Author
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. Thanks for the suggestion! I benchmarked this — The bottleneck is |
||
| task: async () => { | ||
| const tsConfig = ts.findConfigFile('./', ts.sys.fileExists, 'tsconfig.json'); | ||
| if (!tsConfig) { | ||
| throw new Error('Could not find a valid tsconfig.json.'); | ||
| } | ||
|
|
||
| const rootPathsMap = new Map<number, string[]>(); | ||
| await Promise.all( | ||
| roots.map(async (root, idx) => { | ||
| const programPaths = await getProgramPaths(root.config); | ||
| rootPathsMap.set(idx, programPaths); | ||
| }) | ||
| ); | ||
| if (restrictedProgramPaths.length) { | ||
| root.parsedCollections = [...extractCollectors(restrictedProgramPaths, tsConfig)]; | ||
|
|
||
| const rootCollectorMap = new Map<number, string[]>(); | ||
| let allCollectorPaths: string[] = []; | ||
|
|
||
| for (const [idx, programPaths] of rootPathsMap) { | ||
| let paths = programPaths; | ||
|
|
||
| if (typeof restrictProgramToPath !== 'undefined') { | ||
| const restrictProgramToPaths = Array.isArray(restrictProgramToPath) | ||
| ? restrictProgramToPath | ||
| : [restrictProgramToPath]; | ||
| const fullRestrictedPaths = restrictProgramToPaths.map((collectorPath) => | ||
| path.resolve(process.cwd(), collectorPath) | ||
| ); | ||
| paths = paths.filter((p) => fullRestrictedPaths.includes(p)); | ||
| } | ||
|
|
||
| const collectorPaths = filterCollectorPaths(paths); | ||
| rootCollectorMap.set(idx, collectorPaths); | ||
| allCollectorPaths = allCollectorPaths.concat(collectorPaths); | ||
| } | ||
|
|
||
| if (allCollectorPaths.length === 0) { | ||
| return; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| root.parsedCollections = [...extractCollectors(programPaths, tsConfig)]; | ||
| const program = createKibanaProgram(allCollectorPaths, tsConfig); | ||
|
|
||
| for (const [idx, collectorPaths] of rootCollectorMap) { | ||
| roots[idx].parsedCollections = [...extractCollectorsWithProgram(collectorPaths, program)]; | ||
| } | ||
| }, | ||
| title: 'Extracting collectors across all roots', | ||
| }, | ||
| title: `Extracting collectors in ${root.config.root}`, | ||
| })); | ||
| ]; | ||
| } | ||
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.
I think that there's the potential of making this function async (and could cut more time).
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.
Good call —
filterCollectorPathscurrently usesreadFileSyncon all 36,308 globbed files and takes 1.79s. Making it async withfs.promises.readFile+Promise.allwould overlap the I/O and could cut that roughly in half.That said, it's ~3% of the total task time (53s is
createKibanaProgram), so the absolute savings would be ~1s. Happy to do it in a follow-up for cleanliness!