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

Adding a custom separator for the CsvReporter #1197

Merged
merged 2 commits into from
Sep 15, 2017

Conversation

mveitas
Copy link
Contributor

@mveitas mveitas commented Sep 15, 2017

Fixes #1174

Copy link
Member

@arteam arteam left a comment

Choose a reason for hiding this comment

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

Looks good. I've left a suggestion to improve joining.

@@ -220,7 +239,10 @@ private void reportTimer(long timestamp, String name, Timer timer) {
report(timestamp,
name,
"count,max,mean,min,stddev,p50,p75,p95,p98,p99,p999,mean_rate,m1_rate,m5_rate,m15_rate,rate_unit,duration_unit",
"%d,%f,%f,%f,%f,%f,%f,%f,%f,%f,%f,%f,%f,%f,%f,calls/%s,%s",
"%d" + separator + "%f" + separator + "%f" + separator + "%f" + separator + "%f" + separator + "%f" +
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using String.join to avoid repetition? This method uses a StringBuilder under the hood, so it should provide similar performance as concatenation.

this.clock = clock;
this.csvFileProvider = csvFileProvider;

this.histogramFormat = String.join(separator, "%d","%d","%f","%d","%f","%f","%f","%f","%f","%f","%f");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it was nice to have the headers and the format right next to each other in the code, there isn't a need to construct the same value over and for each call to report

@arteam arteam merged commit 6b4df2d into dropwizard:4.0-development Sep 15, 2017
@arteam
Copy link
Member

arteam commented Sep 15, 2017

Thank you for the fixing the issue!

@arteam arteam added this to the 4.0.0 milestone Sep 15, 2017
@mveitas mveitas deleted the csvreporter_delimiter branch September 15, 2017 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants