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.

Sign up for GitHub

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

Jump to bottom

Feat: Included API handling for fetching from daily_node_metrics #71

Merged
merged 5 commits into from
Apr 30, 2024
Merged

Feat: Included API handling for fetching from daily_node_metrics #71

merged 5 commits into from
Apr 30, 2024

Conversation

PatrickNercessian
Copy link
Collaborator

@PatrickNercessian PatrickNercessian commented Apr 12, 2024 •
edited by juliangruber
Loading

@juliangruber juliangruber marked this pull request as draft April 15, 2024 07:02
@PatrickNercessian PatrickNercessian changed the title DRAFT: Included API handling for fetching from daily_node_metrics Feat: Included API handling for fetching from daily_node_metrics Apr 15, 2024
@PatrickNercessian PatrickNercessian marked this pull request as ready for review April 17, 2024 22:35
bajtos
bajtos requested changes Apr 18, 2024
View reviewed changes
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.

I feel it's important to structure the code implementing these "platform stats" endpoints so that it's easy to mirror future updates from spark-stats to voyager-stats repo.

Let's follow the approach we used in filecoin-station/spark-evaluate#188.

I.e. there should be two new files - one for the production code and one for the tests - that can be copied as-is from spark-stats to voyager-stats to mirror the changes.

lib/handler.js Outdated Show resolved Hide resolved
@PatrickNercessian
Copy link
Collaborator Author

I need to update this PR to reflect the change from 'metric_date' to 'day' in filecoin-station/spark-evaluate#188

@PatrickNercessian PatrickNercessian mentioned this pull request Apr 22, 2024
Open
bajtos
bajtos reviewed Apr 25, 2024
View reviewed changes
test/handler.test.js Outdated Show resolved Hide resolved
bajtos
bajtos reviewed Apr 25, 2024
View reviewed changes
lib/handler.js Outdated Show resolved Hide resolved
bajtos
bajtos reviewed Apr 26, 2024
View reviewed changes
lib/handler.js Outdated Show resolved Hide resolved
@bajtos bajtos mentioned this pull request Apr 29, 2024
Merged
bajtos
bajtos requested changes Apr 29, 2024
View reviewed changes
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.

The pull request looks pretty good now! I have a few minor comments to address, PTAL.

lib/handler.js Outdated Show resolved Hide resolved
lib/platform-routes.js Outdated Show resolved Hide resolved
lib/handler.js Outdated Show resolved Hide resolved
test/handler.test.js Outdated Show resolved Hide resolved
test/platform-routes.test.js Outdated Show resolved Hide resolved
bajtos
bajtos approved these changes Apr 29, 2024
View reviewed changes
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 👍🏻

I think we need to wait until filecoin-station/spark-evaluate#188 is landed and Dependabot updates the dependency in package.json, to get the new DB schema migration scripts that will create the new daily_stations table.

spark-stats/package.json

Line 11 in c3ea0b0

"spark-evaluate": "filecoin-station/spark-evaluate#main",

@juliangruber juliangruber merged commit 5ea9490 into filecoin-station:main Apr 30, 2024
6 checks passed
@PatrickNercessian PatrickNercessian deleted the station_id branch April 30, 2024 15:14
@PatrickNercessian PatrickNercessian mentioned this pull request May 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos bajtos approved these changes

@juliangruber juliangruber juliangruber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PatrickNercessian @juliangruber @bajtos