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

Telemetry: Add metadata distinguishing "apps" from "design systems" #30070

Draft
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Dec 15, 2024

Closes #

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-30070-sha-0b45cb37. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-30070-sha-0b45cb37
Triggered by @tmeasday
Repository storybookjs/storybook
Branch tom/metadata-tweaks
Commit 0b45cb37
Datetime Mon Dec 16 05:52:50 UTC 2024 (1734328370)
Workflow run 12346768870

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=30070

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 77.7 MB 77.7 MB 0 B 1.36 0%
initSize 136 MB 136 MB 9.41 kB 1.37 0%
diffSize 58.4 MB 58.4 MB 9.41 kB 1.37 0%
buildSize 7.24 MB 7.24 MB 79 B 1.11 0%
buildSbAddonsSize 1.88 MB 1.88 MB 0 B 1.11 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.86 MB 1.86 MB 29 B 1.07 0%
buildSbPreviewSize 0 B 0 B 0 B - -
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.93 MB 3.93 MB 29 B 1.11 0%
buildPreviewSize 3.3 MB 3.3 MB 50 B 1.76 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 26s 7.3s -18s -709ms -0.93 -255.6%
generateTime 19.2s 22.5s 3.3s 0.95 14.7%
initTime 12.9s 14.8s 1.9s 0.36 12.9%
buildTime 12.8s 10.7s -2s -105ms 0.81 -19.6%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 6.3s 5.3s -1s -9ms 0.08 -19%
devManagerResponsive 4.7s 4s -733ms 0.17 -18.2%
devManagerHeaderVisible 711ms 630ms -81ms 0.32 -12.9%
devManagerIndexVisible 743ms 671ms -72ms 0.19 -10.7%
devStoryVisibleUncached 2.2s 2s -193ms 0.77 -9.6%
devStoryVisible 742ms 672ms -70ms 0.19 -10.4%
devAutodocsVisible 687ms 650ms -37ms 1.44 🔰-5.7%
devMDXVisible 580ms 697ms 117ms 1.44 🔺16.8%
buildManagerHeaderVisible 744ms 634ms -110ms 0.2 -17.4%
buildManagerIndexVisible 862ms 733ms -129ms 0.18 -17.6%
buildStoryVisible 680ms 593ms -87ms 0.27 -14.7%
buildAutodocsVisible 591ms 526ms -65ms 0.58 -12.4%
buildMDXVisible 594ms 516ms -78ms 0.49 -15.1%

Copy link

nx-cloud bot commented Dec 15, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 7a3e991. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@tmeasday tmeasday force-pushed the tom/metadata-tweaks branch from d19abf0 to 766645f Compare December 16, 2024 02:55
@tmeasday tmeasday changed the title Record metadata.hasRouterPackage Telemetry: Add metadata distinguishing "apps" from "design systems" Dec 16, 2024
@tmeasday tmeasday requested a review from shilman December 16, 2024 05:25
@tmeasday tmeasday added maintenance User-facing maintenance tasks telemetry ci:normal labels Dec 16, 2024
});

// If the process errors, this will throw
await process;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i assume we're guaranteed to get all the lines before this exits?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the "promise" doesn't resolve until the command is done. I guess theorectically the final on('line') might fire after that but I've never observed it in practice. I say theorectically just because I don't know for sure either way.

* @param packageJson The package JSON of the project
* @returns Boolean Does this project use a routing package?
*/
export function getHasRouterPackage(packageJson: PackageJson) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasRouterPackage?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the metadata field is called that, I though maybe the helper that gets it should be named differently.

const cache = createFileSystemCache({
basePath: resolvePathInStorybookCache('telemetry'),
ns: 'storybook',
ttl: 24 * 60 * 60 * 1000, // 24h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to lower the TTL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷 I figured we'd thought about it before :)


// We are looking for files with the word "page" or "screen" somewhere in them with these exts
const nameMatches = ['page', 'screen'];
const extensions = ['js', 'jsx', 'ts', 'tsx'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mjs / cjs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are mjsx and cjsx a thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the answer is kinda no: https://www.reddit.com/r/webdev/comments/15mern2/is_there_such_a_thing_as_cjsxmjsx_in_javascript/

And that might be a good reason to not include mjs and cjs in this script -- they are sort of guaranteed to be node files rather than browser files, right? Or is that an incorrect assumption?

@storybook-pr-benchmarking
Copy link

storybook-pr-benchmarking bot commented Dec 16, 2024

Package Benchmarks

Commit: 7a3e991, ran on 16 December 2024 at 06:26:04 UTC

The following packages have significant changes to their size or dependencies:

@storybook/vue3-vite

Before After Difference
Dependency count 111 107 🎉 -4 🎉
Self size 16 KB 16 KB 0 B
Dependency size 45.54 MB 45.46 MB 🎉 -78 KB 🎉
Bundle Size Analyzer Link Link

@storybook/preset-vue3-webpack

Before After Difference
Dependency count 378 374 🎉 -4 🎉
Self size 9 KB 9 KB 0 B
Dependency size 50.58 MB 50.51 MB 🎉 -78 KB 🎉
Bundle Size Analyzer Link Link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:normal maintenance User-facing maintenance tasks telemetry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants