-
Notifications
You must be signed in to change notification settings - Fork 23
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
Added dartCliCommandExecuted
and pubGet
events
#123
Conversation
required List<String> flags, | ||
required List<String> enabledExperiments, |
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.
The types for each of these constructors should be String
, int
, double
, or bool
Additionally, I'm a little concerned with having a list that we convert to a string because we have a 100 char limit for each of these parameters or else they will get truncated. Do you have an idea of how much data could possibly get sent for this one?
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.
Done.
After talking with mit@, I've removed the flags
parameter as it wasn't providing us much value anyway. I think we should be safe to include the set of experiments as there should only ever be a handful enabled at a given time (the full list of experiments can be found here, but many of them are no longer relevant).
/// Event that is emitted when `pub get` is run. | ||
/// | ||
/// [packageName] - the name of the package that was resolved | ||
/// | ||
/// [version] - the resolved, canonicalized package version | ||
/// | ||
/// [dependencyKind] - the kind of dependency that resulted in this package | ||
/// being resolved (e.g., direct, transitive, or dev dependencies). | ||
Event.pubGet({ | ||
required String packageName, | ||
required String version, | ||
required String dependencyType, | ||
}) : eventName = DashEvent.pubGet, | ||
eventData = { | ||
'packageName': packageName, | ||
'version': version, | ||
'dependencyType': dependencyType, | ||
}; |
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.
Just for my awareness here as well, when pub get
is run, there are potentially several packages that get resolved, does this mean that each package will have its own event fired off?
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.
yes, so that could result in hundreds of events. I think we could question the wisdom of this :D
I think it might be worth while updating the USAGE_GUIDE.md
with some guidelines about how many events (and how big events) that can reasonably be sent, and what the performance characteristics look like.
cliCommandExecuted
, pubGet
, and timing
eventscliCommandExecuted
and pubGet
events
cliCommandExecuted
and pubGet
eventsdartCliCommandExecuted
and pubGet
events
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
Required for migration of Dart CLI and dart-lang/pub from
package:usage
topackage:unified_analytics
.