-
Notifications
You must be signed in to change notification settings - Fork 167
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 cron nft-ttr to measure nft time to retrievability #1945
Conversation
93cd4e4
to
45e8abf
Compare
Codecov Report
@@ Coverage Diff @@
## main #1945 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 1259 1259
=========================================
Hits 1259 1259 Continue to review full report at Codecov.
|
0b392a9
to
e6cfff9
Compare
cbdd896
to
b986dab
Compare
Adds an example showing how to store ERC-721 compatible metadata with image data retrieved from a URL on the internet. Adapted from https://github.com/nftstorage/nft.storage/blob/main/examples/client/browser/store.html
This reverts commit 726a7a4.
@alanshaw Should I merge this diff that edits unrelated modules to pass new lint rules into this PR? Or do as followup to this one? I'd slightly prefer the latter. |
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.
Lets get this merged and do the follow up items.
Note: needs a rebase (cron jobs are now integrated with release please so have to be release in the same way as other packages)
"@types/mocha", | ||
"@types/node", | ||
"@types/webpack-env" | ||
] |
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.
Why this nohoist
change? Unless it causing an issue, can we move to a separate PR?
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.
It was causing an issue I believe. I will verify it's really required now.
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.
it wasn't required. :/ ty
export function createRetrievalDurationSecondsMetric(registry) { | ||
/** @type RetrievalDurationSecondsMetric} */ | ||
const metric = new Histogram({ | ||
name: 'retrieval_duration_seconds', |
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 can be follow up
* fix nft-ttr logging and multiple gateways support * remove byteLength label from RetrievalDurationMetric * yarn * yarn * cron.yml action does npm rebuild sharp before test * Revert "cron.yml action does npm rebuild sharp before test" This reverts commit 3d00e0d. * cron ava disble workerThreads because sharp doesn't play nice with them https://sharp.pixelplumbing.com/install#worker-threads * cron workflow runs on pull_request to all branches, not just main * cron: add linearBuckets to retrieval_duration_seconds histogram * measureNftTimeToRetrievability.js starts retrievals in parallel after store so both start right after upload. previous serial method would lead to too-low metrics for the second gateway
Motivation:
Plan
"sade": "^1.7.4"
Possible Futures
.eslintConfig.overrides