-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: enable usage report #270
Conversation
src/usageReport.ts
Outdated
@@ -0,0 +1,55 @@ | |||
import { v4 as uuidv4 } from 'uuid' |
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.
We use self.crypto.randomUUID
in others places in k6 studio, I don't think a separate module is needed
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.
That's on me!
I didn't notice :)
Please send my anonymous usage data to Grafana to aid in | ||
development of k6 Studio.{' '} |
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'd avoid using "please" (as mentioned in Grafana's UX writing guide)
<Text size="2" as="label"> | ||
<Checkbox | ||
{...register('usageReport.enabled')} | ||
defaultChecked |
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.
IMO this is redundant: the field should reflect the current settings: if they are saved in the file, the user will see the value from the file; if not, the user will see the default value
src/usageReport.ts
Outdated
}, | ||
}) | ||
} catch (e) { | ||
console.log('Error sending report', e) |
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.
Since we don't send the report when running the app in dev env, this line isn't really needed. I think we should either log it in the app logs, or fail silently instead
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 fail silently in this case is more appropriate, error from this would just be noise 🤔
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.
LGTM!
Description
This PR introduces changes to collect usage report data when running a k6 script as well as the following when running k6 Studio:
The user can opt-out of the usage report by disabling it in the Settings dialog.
How to Test
usageReport.enabled
property is properly saved--no-usage-report
is passed to the k6 binary (you can do that by logging thek6Args
array in thescript.ts
file.Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Related PR(s)/Issue(s)