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: followup #1945 after testing nft-ttr cron #2000

Merged
merged 11 commits into from
Jun 22, 2022
Merged

Conversation

gobengo
Copy link
Contributor

@gobengo gobengo commented Jun 18, 2022

Motivation:

  • after merging feat: create cron nft-ttr to measure nft time to retrievability #1945 into main so gh actions cron would run it, I took a look https://github.com/nftstorage/nft.storage/actions/runs/2518516959 and found some things to fix
  • the stdout of the gh action job didn't include all logs of what happened. This PR logs them all so we have visibility into what is going on
  • when devving I also noticed that whereas the gh action was trying to pass 2 different ipfs gateways to test retrieving through, only the first was getting used. This PR fixes the sade parsing and cli invocation to pass two gateways through two --gateway {gatewayUrl} flags (space-delimited values used to work with yargs, but not so easy with sade, so for now lets just do one-value-per-flag)
  • remove byteLength label from the prometheus histogram. I'm not sure that it really adds value as it was (e.g. without bucketing). For now I just want to get something useful showing in grafana without as little extra fields as possible, so dont need byteLength.
    • once we have some working dashboard/queries that dont consider byteLength, it probably is a good idea to make that data available to grafana queries. again, I'm just kinda new to prometheus outside of tinkering, and could use some direction/pairing from someone that has dealt with the histograms before
  • fix issue where multiple gateways would be fetched/measured serially. This would mean that gateways other than the first wouldn't measure what we want - their clock wouldn't start until earlier gateways had fully completed. Now the gateways are fetched/retrieved in parallel

@gobengo gobengo requested a review from alanshaw June 18, 2022 00:30
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 18, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 06dea71
Status: ✅  Deploy successful!
Preview URL: https://82383358.nft-storage-1at.pages.dev
Branch Preview URL: https://issues-1943-followup-1.nft-storage-1at.pages.dev

View logs

@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2022

Codecov Report

Merging #2000 (a4bbe05) into main (377b045) will not change coverage.
The diff coverage is n/a.

❗ Current head a4bbe05 differs from pull request most recent head 87972c9. Consider uploading reports for the commit 87972c9 to get more accurate results

@@            Coverage Diff            @@
##              main     #2000   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1259      1259           
=========================================
  Hits          1259      1259           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 530d742...87972c9. Read the comment docs.

@gobengo gobengo marked this pull request as ready for review June 20, 2022 18:08
@gobengo gobengo requested review from alanshaw and removed request for alanshaw June 20, 2022 18:10
@gobengo gobengo added pi/SLA kind/bug A bug in existing code (including security flaws) labels Jun 20, 2022
@gobengo gobengo self-assigned this Jun 20, 2022
… store so both start right after upload. previous serial method would lead to too-low metrics for the second gateway
help: 'How long, in seconds, it took to retrieve an nft image after uploading',
registers: [registry],
labelNames: ['byteLength'],
labelNames: [],
buckets: linearBuckets(0, 0.5, 20),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these buckets should help give some granular measurement to the panels i set up here: #2003

@gobengo gobengo merged commit 0765548 into main Jun 22, 2022
@gobengo gobengo deleted the issues/1943-followup-1 branch June 22, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) pi/SLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants