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 cli options to write profiler output to InfluxDB #17536

Merged
merged 1 commit into from Jul 6, 2017

Conversation

@ferjm
Copy link
Member

ferjm commented Jun 27, 2017

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

This patch adds the command line options and associated code to write the output of running the profiler to an InfluxDB instance, so we can create graphs like [1] with Grafana.

This is part of the work required to record and watch PWM results during CI to catch performance regressions.

[1] https://screenshots.firefox.com/j6sSZrN7pTuPK2kX/localhost


This change is Reviewable

@highfive
Copy link

highfive commented Jun 27, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/profile/lib.rs, components/profile/Cargo.toml, components/profile/time.rs
@ferjm
Copy link
Member Author

ferjm commented Jun 30, 2017

r? @jdm

Copy link
Member

jdm left a comment

If it's useful to put the profiler data into a database, then this looks like a fine way to do it.

/// (`i.e. -p 5`).
/// - a file path to write profiling info to a TSV file upon Servo's termination.
/// (`i.e. -p out.tsv`).
/// - an InfluxDB hostname to store profiling info upon's Servo's termination.

This comment has been minimized.

@jdm

jdm Jun 30, 2017

Member

upon

let database = match *dbname {
Some(ref n) => n,
None => "",
};

This comment has been minimized.

@jdm

jdm Jun 30, 2017

Member
let user = user.as_ref().unwrap_or("");
let password = password.as_ref().unwrap_or("");
let database = dbname.as_ref().unwrap_or("");
let client = create_client(credentials, hosts);

for (&(ref category, ref meta), ref mut data) in &mut self.buckets {
data.sort_by(|a, b| {

This comment has been minimized.

@jdm

jdm Jun 30, 2017

Member

I think you can use data.sort() instead.

This comment has been minimized.

@ferjm

ferjm Jul 3, 2017

Author Member

The trait std::cmp::Ord is not implemented for f64

};
match client.write_one(measurement, None) {
Ok(_) => {},
Err(_) => warn!("Could not write measurement to profiler db"),

This comment has been minimized.

@jdm

jdm Jun 30, 2017

Member
if client.write_one(...).is_err() {
    ...
}
@ferjm ferjm force-pushed the ferjm:influxdb.profiler branch from 7f3a249 to 360f2cc Jul 3, 2017
@ferjm ferjm requested a review from jdm Jul 3, 2017
@ferjm
Copy link
Member Author

ferjm commented Jul 4, 2017

r? @jdm

@jdm
Copy link
Member

jdm commented Jul 6, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Jul 6, 2017

📌 Commit 360f2cc has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 6, 2017

Testing commit 360f2cc with merge 901525c...

bors-servo added a commit that referenced this pull request Jul 6, 2017
Add cli options to write profiler output to InfluxDB

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

This patch adds the command line options and associated code to write the output of running the profiler to an InfluxDB instance, so we can create graphs like [1] with Grafana.

This is part of the work required to record and watch PWM results during CI to catch performance regressions.

[1] https://screenshots.firefox.com/j6sSZrN7pTuPK2kX/localhost

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17536)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 6, 2017

@bors-servo bors-servo merged commit 360f2cc into servo:master Jul 6, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@ferjm ferjm deleted the ferjm:influxdb.profiler branch Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.