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(gatsby): create telemetry service and start it during develop #26832

Merged
merged 5 commits into from Sep 14, 2020

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Sep 9, 2020

As we noticed in #26627, gatsby-telemetry can only run in a node context. In order to add telemetry to Admin we need to create a HTTP server that exposes the telemetry methods for Admin to POST to.

  • Set up express server
  • Expose basic setGatsbyCliVersion and setDefaultComponentId
  • Expose all necessary methods that Admin needs

@mxstbr mxstbr self-assigned this Sep 9, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 9, 2020
@mxstbr mxstbr removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 9, 2020
@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 9, 2020

Gatsby Cloud Build Report

using-styled-components

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 19s

Performance

Lighthouse report

Metric Score
Performance 💚 100
Accessibility 💚 90
Best Practices 💚 100
SEO 💚 90

🔗 View full report

@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 9, 2020

Gatsby Cloud Build Report

client-only-paths

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 20s

Performance

Lighthouse report

Metric Score
Performance 💚 100
Accessibility 🔶 85
Best Practices 💚 100
SEO 🔶 70

🔗 View full report

@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 9, 2020

Gatsby Cloud Build Report

using-reach-skip-nav

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 20s

Performance

Lighthouse report

Metric Score
Performance 💚 100
Accessibility 💚 100
Best Practices 💚 100
SEO 🔶 82

🔗 View full report

@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 9, 2020

Gatsby Cloud Build Report

gatsby

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 21m

@mxstbr mxstbr marked this pull request as ready for review September 9, 2020 10:10
@mxstbr mxstbr requested a review from jamo September 9, 2020 10:10
jamo
jamo previously approved these changes Sep 9, 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.

🤩

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Can we make this part of gatsby-admin and not part of telemetry?

Edit:
Sorry for the fast answer, I didn't want this merged before a discussion and I was heading into a meeting. Moving this to admin would make more sense cause it's the only user. This way we can keep telemetry small with fewer side effects.

Now that I think of it, telemetry itself should probably be a toolkit that allows us to track pieces but gatsby-cli/gatsby should do the setup/flushing, ...

Why did you add it to telemetry and not admin itself?

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 9, 2020

Can we make this part of gatsby-admin and not part of telemetry?
Why did you add it to telemetry and not admin itself?

I put it in gatsby-telemetry because there is nothing Admin-specific in that HTTP server. There will be Admin-specific bits, specifically starting this as a discoverable service for each site, however those will likely have to live in gatsby develop.

I'd be happy to physically move that file to gatsby-admin if you'd prefer that!

@wardpeet
Copy link
Contributor

wardpeet commented Sep 9, 2020

My mistake, please move it to gatsby-develop not gatsby-admin 👍. I just don't want it living in gatsby-telemetry.

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 10, 2020

Done! Ready for a re-review @wardpeet 👍

@mxstbr mxstbr changed the title feat(gatsby-telemetry): create telemetry service feat(gatsby): create telemetry service and start it during develop Sep 11, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Seems to work :) Looking good

@mxstbr mxstbr merged commit 6132b95 into master Sep 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the admin/telemetry-service branch September 14, 2020 08:23
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

3 participants