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

feat(create-gatsby): add telemetry tracking #28107

Merged
merged 7 commits into from
Nov 17, 2020
Merged

feat(create-gatsby): add telemetry tracking #28107

merged 7 commits into from
Nov 17, 2020

Conversation

mfrachet
Copy link
Contributor

Description

Add telemetry to create-gatsby

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 16, 2020
@mfrachet mfrachet added topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 16, 2020
@mfrachet mfrachet requested review from jamo and a team November 16, 2020 17:29
@mfrachet mfrachet marked this pull request as ready for review November 16, 2020 17:29
Copy link
Contributor

@gillkyle gillkyle left a comment

Choose a reason for hiding this comment

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

Amazing how straightforward this is, but the value from it will be huge 🙂

packages/create-gatsby/src/index.ts Show resolved Hide resolved
@ascorbic
Copy link
Contributor

OK, I don't think we can merge this without some fixes to the gatsby-telemetry package. We need the download size for this to stay small, and the telemetry package is very big. @jamo is there any way we can reduce the size of it? Does it need the whole of lodash?

@jamo
Copy link
Contributor

jamo commented Nov 16, 2020

@ascorbic lodash.merge is the only one we use afaik, for deep-merging objects (so Object.assign won't work (unless there is a config option))

@ascorbic
Copy link
Contributor

@jamo A thought: is it possible for us to have a lighter-weight approach, where we just enqueue the events at this stage, and then actually dispatch them when gatsby itself has been installed? I'm trying to think of ways that we could avoid needing to depend on the whole telemetry package.

@ascorbic
Copy link
Contributor

We could replace lodash.merge with deepmerge if that's all we need. That's less than 1kB

@jamo
Copy link
Contributor

jamo commented Nov 17, 2020

I don't think it would make sense to implement a custom library for this purpose, rather invest the time to optimize the bundle size. Maybe we could ncc everything 🤔

@jamo
Copy link
Contributor

jamo commented Nov 17, 2020

I was originally against lodash but I was told that I need to use it and the size isn't a concern as gatsby already ships with it :(

@ascorbic
Copy link
Contributor

Yeah, it's fine in gatsby, but the considerations in create-gatsby are very different. For context: the whole compiled package is around 60kB at the moment. This PR would quadruple that. I'm not sure how realistic it is to shrink it enough (nor if that's a valuable use of time, either). If we could extract just the queuing part, we could avoid needing all the network stuff.

@jamo
Copy link
Contributor

jamo commented Nov 17, 2020

I wouldn't feel too comfortable maintaining two packages doing the same work, the risk of losing or misformatting data becomes significant. I should have time next week to look at the telemetry-package bundle size. But sounds like you have already a plan on how to drop lodash so why won't we start with that?

jamo
jamo previously approved these changes Nov 17, 2020
Copy link
Contributor

@jamo jamo left a comment

Choose a reason for hiding this comment

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

👌 :shipit:

Thanks!

const analyticsApi =
process.env.GATSBY_TELEMETRY_API || `https://analytics.gatsbyjs.com/events`

let machineId = store.get(`telemetry.machineId`)
Copy link
Contributor

Choose a reason for hiding this comment

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

We might need to move this to be after the function

Suggested change
let machineId = store.get(`telemetry.machineId`)
let machineId = getMachineId()

method: `POST`,
headers: {
"content-type": `application/json`,
"user-agent": `Gatsby Telemetry`,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit nit nit, we could maybe add the package version here. just in case we see one acting up

jamo
jamo previously approved these changes Nov 17, 2020
valueStringArray?: Array<string>
}

export const trackCli = (eventType: string, args?: ITrackCliArgs): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sweet! Didn't realise it was so simple.

ascorbic
ascorbic previously approved these changes Nov 17, 2020
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

This is great! Nice work. I would like to get the machine info in at some point, but it's not a blocker

@mfrachet mfrachet dismissed stale reviews from ascorbic and jamo via f21a91d November 17, 2020 16:55
jamo
jamo previously approved these changes Nov 17, 2020
Copy link
Contributor

@jamo jamo left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants