feat: analytics - #917
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
7cc8437 to
05172ac
Compare
|
@dominoweir Is this the kind of thing you were thinking, that we'd grab the cc: @kjsutherland |
05172ac to
b3f94ec
Compare
dominoweir
left a comment
There was a problem hiding this comment.
thanks, this is precisely what I was thinking!
| export function trackEvent(event: string) { | ||
| if (import.meta.env.DEV && import.meta.env.VITE_LOG_ANALYTICS) { | ||
| console.log(`Track Event: ${event}`); | ||
| } | ||
|
|
||
| postAnalyticsEvent(event); |
There was a problem hiding this comment.
I like this added logging- I'm guessing that dev environment variable is specific to this visualizer? if not we should consider moving this to web/core
There was a problem hiding this comment.
We use of .env for a few things like this, from logging out the board from the game object to picking a replay file to preview with, so yep it's specific here just now.
We'd generally leave this kind of logging as a project specific thing, as we'll log just what we're sending to our trackEvent function formatting it to be easier to read in the console and omitting common stuff that's repeated across all the final tracking calls that we're not so interested in when while we're implementing and testing it.
But as the data in the tracking events are nice and simple how you've set it up having in in core would totally work too.
| @@ -0,0 +1,9 @@ | |||
| import { postAnalyticsEvent } from '../../../../../../../../../web/core/src/analytics'; | |||
There was a problem hiding this comment.
ah I'm guessing I forgot to export this and hence it doesn't show up as an export from @kaggle-environments/core, I can fix that real quick
There was a problem hiding this comment.
oh wait it's exported- you should be able to do import { postAnalyticsEvent } from "@kaggle-environments/core" which is a bit cleaner
There was a problem hiding this comment.
That's much nicer, my bad I should have spotted that and tidied it up. Thanks!
I'm fine with it just being |
3aad564 to
15b82b9
Compare
When I log out So our case it's currently Now the transformer is in our visualizer's code I seem to be able to change this without anything breaking. I'm not sure if it's still used anywhere apart from your new Is this correct @dominoweir that we can change this without effecting anything else now? So I'm thinking we need to make sure our tracking calls set
|
Totally agree with this take- I think for sanity purposes as we scale up the number of games, having confidence that the name we should look up is the same as the name in replays will be great. |
Adding simple event tracking, using the shared
postAnalyticsEvent()function.Currently triggers for all of the events originally mentioned in the doc shared with @dominoweir.
But I know some of these are not so interesting for the Kaggle team so might be taken out, it was more to try out how the naming events would work and if simple strings for each are ok. Or if using the ability to pass in an object to
postAnalyticsEventand more structured events would be handy.To make it easier to check if the events are triggering rather than having to watch the parent window for messages being posted , added logging that can be enabled in
.envwithVITE_LOG_ANALYTICS.Just currently not sending a
gameproperty topostAnalyticsEvent, should this begoorgo-v2?