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

No namespace in export filenames can lead to data loss #529

Closed
jskeet opened this Issue Aug 19, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@jskeet

jskeet commented Aug 19, 2017

Yesterday I added benchmarks to NodaTime for a type, NodaTime.TimeZones.StandardDaylightAlternatingMap. I put the benchmarks in the NodaTime.Benchmarks.NodaTimeTests.TimeZones namespace.

As it happens, I already had some benchmarks for that type, in the NodaTime.Benchmarks.NodaTimeTests namespace. When I looked at the results of my overnight run, only the new tests existed. That's because the JSON file that's generated only includes the short type name, with no namespace.

That's probably great for cases when you're looking at results by hand and don't want long filenames, but in my case I'd rather have long filenames (which are being consumed automatically anyway) and no loss of data.

Perhaps this should be an exporter option?

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin

AndreyAkinshin Aug 20, 2017

Member

Hey @jskeet, thanks for the report.

I think we always should use fully qualified names for all benchmarks without additional exporter options.

Member

AndreyAkinshin commented Aug 20, 2017

Hey @jskeet, thanks for the report.

I think we always should use fully qualified names for all benchmarks without additional exporter options.

@Teknikaali

This comment has been minimized.

Show comment
Hide comment
@Teknikaali

Teknikaali Sep 16, 2017

Contributor

I dug around the code and this is what I discovered about exported files' naming conventions:

Summary.Title property is being used for both the filename and the actual title of the benchmark.
Currently Summary.Title is generated by using Type.Name, but it could be change to Type.FullName to enable fully qualified filenames.

This change would cause a small side effect to the actual output data by making the Title value a bit longer. If this is a wanted result then it's ok and nothing to worry about.. The results of this change would look like this:

Property used Filename Example JSON output snip
Type.Name IntroExporters-report-brief.json "Title":"IntroExporters",
Type.FullName BenchmarkDotNet.Samples.Intro.IntroExporters-report-brief.json "Title":"BenchmarkDotNet.Samples.Intro.IntroExporters",

I can start fixing this, if these changes look good to you!

Contributor

Teknikaali commented Sep 16, 2017

I dug around the code and this is what I discovered about exported files' naming conventions:

Summary.Title property is being used for both the filename and the actual title of the benchmark.
Currently Summary.Title is generated by using Type.Name, but it could be change to Type.FullName to enable fully qualified filenames.

This change would cause a small side effect to the actual output data by making the Title value a bit longer. If this is a wanted result then it's ok and nothing to worry about.. The results of this change would look like this:

Property used Filename Example JSON output snip
Type.Name IntroExporters-report-brief.json "Title":"IntroExporters",
Type.FullName BenchmarkDotNet.Samples.Intro.IntroExporters-report-brief.json "Title":"BenchmarkDotNet.Samples.Intro.IntroExporters",

I can start fixing this, if these changes look good to you!

@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Sep 17, 2017

Member

@Teknikaali IMHO the best option would be to leave the Summary.Title untouched and use the benchmar.Target.FullName when creating files

@AndreyAkinshin what do you think?

Member

adamsitnik commented Sep 17, 2017

@Teknikaali IMHO the best option would be to leave the Summary.Title untouched and use the benchmar.Target.FullName when creating files

@AndreyAkinshin what do you think?

@Teknikaali

This comment has been minimized.

Show comment
Hide comment
@Teknikaali

Teknikaali Sep 17, 2017

Contributor

@adamsitnik I think you're right, it should work nicely without any side effects if the Benchmark.Target.Type.FullName property would be used. I totally missed this one!

This then leaves only one more question about the filename handling inside the ExporterBase class:
BenchmarkRunnerCore.GetTitle(...) is a method that parses the title from a collection of Benchmarks.
If only targets of a same type exists in the collection, that type's name will be used as the title.
But if multiple types of Benchmarks exists, then a custom title will be returned instead:

return $"BenchmarkRun-{benchmarkRunIndex:##000}-{DateTime.Now:yyyy-MM-dd-hh-mm-ss}";

That custom title is apparently also being used as the filename when exporting, so should I include some kind of check like this inside ExporterBase ?

if (multiple benchmark types)
    use custom title from Summary.Title
else
    use fully qualified type name from Benchmarks-collection's first benchmark.Target.Type.FullName
Contributor

Teknikaali commented Sep 17, 2017

@adamsitnik I think you're right, it should work nicely without any side effects if the Benchmark.Target.Type.FullName property would be used. I totally missed this one!

This then leaves only one more question about the filename handling inside the ExporterBase class:
BenchmarkRunnerCore.GetTitle(...) is a method that parses the title from a collection of Benchmarks.
If only targets of a same type exists in the collection, that type's name will be used as the title.
But if multiple types of Benchmarks exists, then a custom title will be returned instead:

return $"BenchmarkRun-{benchmarkRunIndex:##000}-{DateTime.Now:yyyy-MM-dd-hh-mm-ss}";

That custom title is apparently also being used as the filename when exporting, so should I include some kind of check like this inside ExporterBase ?

if (multiple benchmark types)
    use custom title from Summary.Title
else
    use fully qualified type name from Benchmarks-collection's first benchmark.Target.Type.FullName
@adamsitnik

This comment has been minimized.

Show comment
Hide comment
@adamsitnik

adamsitnik Sep 17, 2017

Member

@Teknikaali yes, this looks like the best option

big thanks for doing this!

Member

adamsitnik commented Sep 17, 2017

@Teknikaali yes, this looks like the best option

big thanks for doing this!

@AndreyAkinshin

This comment has been minimized.

Show comment
Hide comment
@AndreyAkinshin
Member

AndreyAkinshin commented Sep 18, 2017

@Teknikaali, LGTM

@adamsitnik adamsitnik added this to the v0.10.10 milestone Sep 25, 2017

alinasmirnova added a commit to alinasmirnova/BenchmarkDotNet that referenced this issue Sep 22, 2018

Fix exporters to use fully qualified filenames (dotnet#552), fixes do…
…tnet#529

* Fix exporters to use fully qualified filenames

* Fix long integration test run durations

alinasmirnova added a commit to alinasmirnova/BenchmarkDotNet that referenced this issue Sep 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment