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

Add (perf_data) upload to CLI report subcommand #146

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brainstorm
Copy link

@brainstorm brainstorm commented Jun 14, 2023

As discussed offline, adding a CLI subcommand to submit firefox's profiler-server data, example viz here.

Work in progress, I was unsure about:

  1. In which CLI subcommand to drop this functionality, I hope report is a good place? The other candidate was perf, happy to move it there if you think it's more appropriate?
  2. Should I POST a report from the CLI and then have the bencher backend forward the POST of the received data to localhost:5252/compressed-data? OR have the CLI directly post it to the profile-server endpoint? I suspect that you want the former for several reasons... OTOH, the forwarding shuffles quite a few bytes on a couple of hops unless we send a 302 Redirect?

Anyway, let me know in advance if there's something off in my thinking or implementation so that we are on the same page, @epompeii.

Depends on #127

@brainstorm
Copy link
Author

brainstorm commented Jun 14, 2023

Thinking about point 2 above... you wanted to host the firefox profile-server at bencher.dev or were you planning use the public instance instead (https://api.profiler.firefox.com/)?... ahh, re-reading your email it seems that you want the latter, so that means having a direct POST of the bencher CLI to that endpoint? Easy!

@epompeii
Copy link
Member

epompeii commented Jun 14, 2023

Thank you for taking the initiative on this, @brainstorm!

Yes, for the first pass lets just use the public instance (https://api.profiler.firefox.com). Long term, all this functionality should be built into the Bencher instead of adding an external dependency, so we'll just keep it simple for now.

I was thinking that this wouldn't be a sub-command. Rather it would be an optional argument to bencher run (which is also aliased to bencher report create). Something along the lines of --profile that would be an Option<PathBuf>.
In the bencher run code, this would read the profile from the specified path after all benchmarks have been run but before the report has been posted to the API backend.
Basically right here: https://github.com/bencherdev/bencher/blob/main/services/cli/src/bencher/sub/project/run/mod.rs#L146

To keep this from being hard blocked on #127 you can just do all the work here and store it as let _profile = ... for now. Once I get the metadata added to the report, I'll just drop it in there!

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 this pull request may close these issues.

2 participants