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

Provide CSV measurements export with filtering/pagination #676

Closed
sasharevzin opened this issue May 9, 2020 · 16 comments
Closed

Provide CSV measurements export with filtering/pagination #676

sasharevzin opened this issue May 9, 2020 · 16 comments
Assignees

Comments

@sasharevzin
Copy link
Collaborator

I found that it exports all measurements into CSV https://github.com/Safecast/safecastapi/blob/master/app/controllers/measurements_controller.rb#L58
Of course, the query always timeout.

@matschaffer
Copy link
Contributor

Csv is kind of important for researchers. Would be good to have some sort of csv data available. Even if it has to be periodic exports to s3 rather than queryable.

@sasharevzin
Copy link
Collaborator Author

If we have somewhere the CSV dump at s3, then we can just redirect to it. Currently, I don't see any export measurements to CSV file so I guess no one is using. Possible?

@matschaffer
Copy link
Contributor

Could be. How did you come across the code?

@sasharevzin
Copy link
Collaborator Author

In a same controller for this issue #529

@sasharevzin
Copy link
Collaborator Author

@matschaffer @auspicacious. I think it makes sense to remove this option. It just adds pressure to db.

@auspicacious
Copy link
Contributor

Is the daily export listed on https://github.com/Safecast/safecastapi/wiki/Data-Sets in CSV format? If so, then I agree with @sasharevzin

(P.S. the page should document the format)

@seanbonner
Copy link
Member

Just to reiterate @matschaffer's comment, the CSV file option is really important to researchers, so if this is redundant because it's already happening somewhere else then it's OK to remove, but if this is what is generating the CSV we publish everyday then it's really very important to keep.

@sasharevzin
Copy link
Collaborator Author

@matschaffer
Copy link
Contributor

Okay, after taking a closer look at what you mean, it's specifically https://api.safecast.org/en-US/measurements?format=csv that exists, but doesn't work since it doesn't include any filtering or pagination.

I'll re-word the description to add that.

To be fair, it's broken and no-one has mentioned anything so it probably doesn't see much use, but to @seanbonner 's point CSVs are important so we shouldn't just drop the support, we should try to improve it.

@matschaffer matschaffer changed the title Do we still need export to CSV? Provide CSV measurements with filtering Jun 14, 2020
@matschaffer matschaffer changed the title Provide CSV measurements with filtering Provide CSV measurements export with filtering/pagination Jun 14, 2020
@matschaffer
Copy link
Contributor

Heh, in it's current form it basically just kills the DB, so on second thought I'll open two tickets, one to remove the current support, and another add it back in a way that doesn't try to export everything.

@matschaffer
Copy link
Contributor

I'm inclined to leave this as is.

It'd be great to have some sort of "slow query" option for generating large csvs asynchronously, but in the mean time I don't think we should just remove what's there.

Folks who know how it can be used, can use it. Folks who don't will get an error. Probably good enough until we have a better story on providing large CSV blobs ad-hoc.

@sasharevzin
Copy link
Collaborator Author

@matschaffer but if we will add pagination to CSV export then everything will be fine. Just saying :)

@matschaffer
Copy link
Contributor

yeah, or some reasonable limit could be worth trying. Though sometimes psql does weird things with limit queries.

@sasharevzin sasharevzin linked a pull request Jun 15, 2020 that will close this issue
@sasharevzin
Copy link
Collaborator Author

@matschaffer Added back filters and pagination

@sasharevzin
Copy link
Collaborator Author

PR closed

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

Successfully merging a pull request may close this issue.

4 participants