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

analytics: tag tilt.up.running w/ subcommand #3560

Merged
merged 1 commit into from Jul 7, 2020

Conversation

landism
Copy link
Member

@landism landism commented Jul 7, 2020

Problem

The tilt.up.running event also gets emitted by tilt ci, and includes no information to distinguish tilt ci from tilt up.

Solution

Add a tag for the subcommand (e.g., "up", "ci") on tilt.up.running events.

Notes

  • We should rename tilt.up.running as well, but that makes this change riskier (since it gives us an inconsistency to account for in our analytics queries) and can be decoupled from this change.
  • Ideally we have a separate mechanism to filter out runs in CI from human runs and this isn't the only thing we rely on, but:
    • that's always going to be a heuristic situation and more signal is better
    • this might be nice to have for other reasons
  • Perhaps it'd be better add this as a global tag rather than just on this one count, but that's probably a couple hours of work instead of a few minutes.

@landism landism requested review from nicks and maiamcc July 7, 2020 15:26
@landism landism merged commit 63e1d0c into master Jul 7, 2020
@landism landism deleted the matt/up.running_report_subcommand branch July 7, 2020 17:19
@maiamcc
Copy link
Contributor

maiamcc commented Jul 7, 2020

CC @victorwuky: when providing metrics to customers derived from tilt.up.running, can filter on subcommand: up to make sure we're just giving them metrics related to tilt up calls (and/or can use the subcommand field to see if anyone is running tilt ci, etc.)

@maiamcc
Copy link
Contributor

maiamcc commented Jul 7, 2020

thanks for this @landism !!

@wu-victor
Copy link
Contributor

Thanks @maiamcc @landism . I'll be sure to try this out once we have a new release.

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

4 participants