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

update getStorybookMetadata to safely record version even if fails to parse mainConfig #753

Conversation

JonathanKolnik
Copy link
Contributor

@JonathanKolnik JonathanKolnik commented Jun 2, 2023

Even though we reproduced, tested, and fixed a mainConfig parsing bug, it is possible that certain formats are still not readable. In that event we want to at least still record the metadata that we can gather.

@linear
Copy link

linear bot commented Jun 2, 2023

AP-2756 Chromatic telemetry reports `null` `storybook_version`

How is the user affected?

Our telemetry reports storybook_version = null for a large proportion of our build events:

image.png

How many and/or what class of users does this impact?

Unsure, possible all users, or maybe a specific CLI version / SB version

Is there a workaround?

Backfill data based on other data.

What are the steps for reproducing the issue?

Not sure.

Other information

Run this query to see the above data (run on Jan 18):

SELECT count(*) as count, storybook_version FROM `warehouse-production-209019.chromatic_webapp2.create_ci_build_view`
WHERE extract(year from timestamp) = 2023
GROUP BY storybook_version
ORDER BY count DESC

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

OK, LGTM. @zol @thafryer this will mean that in some proportion of cases (it looks like maybe as many as 1/3) where we fail to parse main.js, we report the SB + CH versions correctly, but we won't have a valid addons list or builder (they'll be "unknown").

In order to investigate further we'll probably need to start uploading the errors we are getting trying to parse the file. That's a bit tricky because we need to sanitize the errors etc. We should consider if we want the data enough to do that.

Copy link
Member

@thafryer thafryer left a comment

Choose a reason for hiding this comment

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

LGTM. Tests are failing. It's likely because of the tweak to catch block.

bin-src/lib/getStorybookMetadata.ts Outdated Show resolved Hide resolved
Copy link
Member

@thafryer thafryer left a comment

Choose a reason for hiding this comment

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

LGTM. Tests are failing. It's likely because of the tweak to catch block.

JonathanKolnik and others added 3 commits June 7, 2023 12:18
Co-authored-by: Jarel Fryer <68470411+thafryer@users.noreply.github.com>
…chromatic-telemetry-reports-null-storybook_version-2
@tmeasday tmeasday merged commit 1fbe85e into main Jun 8, 2023
18 of 19 checks passed
@tmeasday tmeasday deleted the jono/ap-2756-chromatic-telemetry-reports-null-storybook_version-2 branch June 8, 2023 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants