Add event processor functionality #2420
Conversation
|
Any reason why |
|
There was a problem hiding this comment.
Overall it looks good, but let's iterate on simplifying it a bit and unifying the naming.
Thank you for your contribution @caaavik-msft !
There was a problem hiding this comment.
We are almost there, thank you for addressing the feedback @caaavik-msft !
src/BenchmarkDotNet/EventHandlers/CompositeBenchmarkEventHandler.cs
Outdated
Show resolved
Hide resolved
caaavik-msft
changed the title
|
@adamsitnik are we good to merge this in now? |
There was a problem hiding this comment.
LGTM, but please rename the type before I hit the merge button.
Thank you @caaavik-msft !
| public virtual void OnStartBuildStage() { } | ||
| public virtual void OnBuildFailed(BenchmarkCase benchmarkCase, BuildResult buildResult) { } | ||
| public virtual void OnStartBuildStage(IReadOnlyList<BuildPartition> partitions) { } | ||
| public virtual void OnBuildComplete(BuildPartition benchmarkCase, BuildResult buildResult) { } | ||
| public virtual void OnEndBuildStage() { } |
There was a problem hiding this comment.
what is the difference between OnBuildComplete and OnEndBuildStage? The first is for one partition and the latter is for all of them?
There was a problem hiding this comment.
Yes, that is the difference between them.
| @@ -221,6 +224,12 @@ public ManualConfig AddLogicalGroupRules(params BenchmarkLogicalGroupRule[] rule | |||
| return this; | |||
| } | |||
|
|
|||
| public ManualConfig AddEventProcessor(EventProcessor eventProcessor) | |||
There was a problem hiding this comment.
nit: it's unlikely that the users will need more than one instance
While this may be true, this makes it the only one of the Add... methods that doesn't accept a params array. Personally, I would prefer the API consistency.
|
Also, |
|
@caaavik-msft Can you address @mawosoft's comment in another PR (merging configs)? |
This addresses #2054 which is a feature request to have hooks which can enable other tools that are invoking BDN to display progress rather than having to parse live console output or to wait for all the benchmarks to complete to have the exporters run.
A full proposal and explanation of the events/hooks in this PR are covered in this comment: #2054 (comment)
There are potentially many more hooks that could be added like events for when a benchmark run enters a different stage, but they would likely need to be added as custom events sent through the Broker and supported by all the different executors/toolchains, which I didn't want to add at this stage. I'm open as well to modifying or adding extra events before merging this in to see what people think would be useful.
I've also included a
EventHandlerBaseclass which is a base class that implements all the hooks as virtual methods, so that implementing classes only need to override the hooks they care about.