Skip to content

Description replaces Method name column in exporter #1243

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

Open
dotMorten opened this issue Sep 12, 2019 · 5 comments
Open

Description replaces Method name column in exporter #1243

dotMorten opened this issue Sep 12, 2019 · 5 comments

Comments

@dotMorten
Copy link

dotMorten commented Sep 12, 2019

If I set a description on a benchmark, I lose the method name in the generated CSV.
The method name is in a way the unique identifier of the benchmark, whereas a description is merely describing what the benchmark is meant to test, and not guaranteed to be unique either.
Could we instead of having a single "Method" column, have two: Method and Description so the output could have both? It becomes rather important when storing daily benchmarks in a database that the "ID" of the benchmark stays unique and consistent, but a description can be allowed to change any time.

I assume this behavior is caused by this line:

WorkloadMethodDisplayInfo = FormatDescription(description) ?? workloadMethod?.Name ?? "Untitled";

But it's misleading in the report output to have the description listed under "Method"

@dotMorten dotMorten changed the title Description overrides method column in exporter Description re[;aces Method name column in exporter Sep 12, 2019
@dotMorten dotMorten changed the title Description re[;aces Method name column in exporter Description replaces Method name column in exporter Sep 12, 2019
@AndreyAkinshin
Copy link
Member

Hey @dotMorten, thanks for the report!

Originally, Description was introduced as a way to "override" the method name in the summary table with a custom string. However, your concerns sound pretty reasonable, so I suggest the following changes:

  • The "Method" column should be renamed to "Benchmark", it will help to avoid Method vs. Description confusion. By default, I suggest keeping the existing behavior and display only once column.
  • We can introduce a parameter in SummaryStyle that allows splitting the "Benchmark" column into two "Method" and "Description" column to keep the summary table as small as possible.
  • All raw exporters (e.g., the Csv exporter) should contain the original method name in the "Method" column. The optional "Description" column should be added when some benchmarks have descriptions.

What do you think?

@dotMorten
Copy link
Author

I think that's a good approach. I tried making my own subclass of the exporter, but so much is relying on logic deep down in the summary I found it rather hard to write a custom set of columns out - especially since the columns change with the number of parameters, loggers etc.

@roji
Copy link
Member

roji commented Apr 17, 2020

The method name is in a way the unique identifier of the benchmark

For uniqueness shouldn't both the name and class be considered?

In any case, I have the opposite need, and was indeed treating Description as a way to override the method name. This is the case, for example, when extending a benchmarks base class, and not wanting the subclass to have different names. So IMHO it would be good to have a Benchmark column, which defaults to the method name but can be overridden with an attribute (currently Description, but at some point this could be changed as a breaking change). This could be expected to be unique (within the class!), and if the user overrides it, it's their responsibility to make sure it remains so. I personally don't see a huge need for an arbitrary textual description, but why not.

It's useful to compare unit test framework here, where the test name typically defaults to the method name but is still user-configurable.

@dotMorten
Copy link
Author

Sure. The main thing here is that the generated CSV format is quite useless for importing into a database to track performance over time. You're likely to update the description, so suddenly tests wouldn't match, and uniqueness isn't guaranteed.
(the other problem is the custom parameters are in the middle of the CSV so hard to figure out what are real values and what are custom - I would have preferred they are at the end of the generated CSV so the rest of the schema stays consistent).

@roji
Copy link
Member

roji commented Apr 19, 2020

You're likely to update the description

That depends... If you're using it simply as a way to define the benchmark name that isn't the method name, and you care about keeping names constant over time (because you're putting them into the database), then they indeed shouldn't be changed...

This seems to be more a problem of naming - "Description" does usually imply a user-displayed text that isn't important for any program and can change at any time. If this were renamed to "Name" things would make more sense, and I'm not sure anyone actually needs a user-displayable Description separate from a simple Name (am comparing with unit testing frameworks, where that doesn't typically exist).

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

No branches or pull requests

3 participants