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

chore(gatsby-plugin-benchmark-reporting): Add more meta data #21369

Merged
merged 2 commits into from
Feb 13, 2020
Merged

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Feb 11, 2020

This adds a few new fields to the benchmark plugin: git repo hash, ci status, siteId, cwd, ci name, node version, gatsby version, gatsby cli version, sharp version (if present), webpack version, node memory (straight dump of process.memoryUsage()), size of all js files in root folder of public, and the number of images found recursively in public or .cache.

It also groups the versions and counts into a struct.

The plugin now reports the text of a 500 error, if any.

Need to update big query schema before pulling this. @jamo has updated the schema accordingly.

This change is not windows friendly due to the execSyncs using find, wc, bc, and forward slashes. There are probably one or two more subtleties that I can't test for. For the time being this plugin is intended to run with benchmarking service and some CI's, which all run on linux. If we need windows support, we can iterate (most-if-not-all of this stuff can be done with just JS, just more work).

Additionally the execs are sync because that just makes it easier. There are only a handful, and there is no performance concern because this runs at the end of a run. Changing it to async would not make a relevant difference but more annoying code to maintain.

$ BENCHMARK_REPORTING_URL=https://secret yarn bench
...
info Gathered data: {
  "time": "2020-02-13T12:21:21.207Z",
  "sessionId": "ed4fe1f3-afb3-4314-a493-f40715c3d5b9",
  "cwd": "/home/foo/gatsby/ifsc-150k",
  "timestamps": {
    "bootstrapTime": 1551,
    "instantiationTime": 1551,
    "benchmarkStart": 1594,
    "preInit": 1594,
    "preBootstrap": 1682,
    "preBuild": 2436,
    "postBuild": 7874,
    "benchmarkEnd": 7874
  },
  "gitHash": "cfa8e65c60b868bc5034b7c82f9330eb572dbc8c",
  "ci": false,
  "ciName": "local",
  "versions": {
    "nodejs": "v10.18.1",
    "gatsby": "2.19.16-dev-1581506531816",
    "gatsbyCli": "Gatsby CLI version: 2.8.29\nGatsby version: 2.19.16-dev-1581506531816\n  Note: this is the Gatsby version for the site at:/home/foo/gatsby/ifsc-150k",
    "sharp": "none",
    "webpack": ""
  },
  "counts": {
    "pages": 13,
    "jpgs": 0,
    "pngs": 0,
    "gifs": 0,
    "other": 0
  },
  "memory": {
    "rss": 322387968,
    "heapTotal": 247218176,
    "heapUsed": 157750664,
    "external": 151727365
  },
  "publicJsSize": 232929
}
info Flushing benchmark data to remote server...
info Server response: 201: Created

@pvdz pvdz added the topic: performance Related to runtime & build performance label Feb 11, 2020
@pvdz
Copy link
Contributor Author

pvdz commented Feb 11, 2020

(Requests for feedback were meant as a ping, but feedback is welcome. This cannot merge until db is updated to support the new fields.)

@pvdz
Copy link
Contributor Author

pvdz commented Feb 11, 2020

Hm, should it also report the size of images? As a rough guide for triage of why image processing took so long or whatever?

nicholascapo
nicholascapo previously approved these changes Feb 11, 2020
Copy link
Contributor

@nicholascapo nicholascapo left a comment

Choose a reason for hiding this comment

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

Still need CI_NAME

@pvdz pvdz marked this pull request as ready for review February 13, 2020 12:26
@pvdz pvdz requested a review from a team as a code owner February 13, 2020 12:26
@pvdz pvdz force-pushed the bench-meta branch 2 times, most recently from 1706b55 to 976f899 Compare February 13, 2020 12:49
@pvdz
Copy link
Contributor Author

pvdz commented Feb 13, 2020

@nicholascapo after some discussion we're pulling the siteId explicitly from the analytics object for now. If in the future more things get added to this env object it we can update accordingly to various implementations. Mainly wanted to prevent the footgun where the analytics object gets a new field while the benchmarking schema isn't expecting it and the server just refuses it.

@pvdz
Copy link
Contributor Author

pvdz commented Feb 13, 2020

The siteId will be "error" if the tags value set and failed to parsed as json, just so we can track that just in case. It's empty if not set at all and should be the value supplied otherwise.

@pvdz pvdz merged commit 9d3a8f0 into master Feb 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the bench-meta branch February 13, 2020 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: performance Related to runtime & build performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants