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: metric for successful http retrievals #437

Merged
merged 7 commits into from
Jan 8, 2025

Conversation

NikolasHaimerl
Copy link
Contributor

@NikolasHaimerl NikolasHaimerl commented Jan 7, 2025

This MR proposes the following changes:

  1. Add a metric for counting the successful http retrievals

Following the definition of this issue the metric is defined as:

how many retrievals succeeded using HTTP protocol for CAR transfer. (A retrieval that had to use Graphsync because the SP does not advertise HTTP is considered as a failure.)

In practice a successful http request is defined by its sucess indicated by Measurement.retrievalResult as well as the protocol being used indicated byMeasurement.protocol.

@NikolasHaimerl NikolasHaimerl marked this pull request as ready for review January 8, 2025 07:42
@NikolasHaimerl NikolasHaimerl requested a review from bajtos January 8, 2025 07:42
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Great start! Let's improve the tests a bit.

test/retrieval-stats.test.js Outdated Show resolved Hide resolved
test/retrieval-stats.test.js Outdated Show resolved Hide resolved
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Almost there!

test/retrieval-stats.test.js Outdated Show resolved Hide resolved
test/retrieval-stats.test.js Outdated Show resolved Hide resolved
NikolasHaimerl and others added 2 commits January 8, 2025 10:41
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM! 👏🏻

test/retrieval-stats.test.js Show resolved Hide resolved
@bajtos
Copy link
Member

bajtos commented Jan 8, 2025

@NikolasHaimerl Please go ahead and land the pull request yourself.

Our GitHub Actions CI/CD pipeline will automatically deploy this change to Fly.io. After the current Spark round ends and spark-evaluate finishes the evaluation run, you will see the first new data point in InfluxDB.

@NikolasHaimerl NikolasHaimerl enabled auto-merge (squash) January 8, 2025 10:27
@NikolasHaimerl NikolasHaimerl merged commit 679265d into main Jan 8, 2025
6 checks passed
@NikolasHaimerl NikolasHaimerl deleted the nhaimerl-add-http-rsr-influxdb branch January 8, 2025 10:35
Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ done
Development

Successfully merging this pull request may close these issues.

3 participants