cloud-bulldozer / benchmark-wrapper

Python Library to run benchmarks
https://benchmark-wrapper.readthedocs.io
Apache License 2.0
19 stars 56 forks source link

Create Signal Protocol #295

Open learnitall opened 2 years ago

learnitall commented 2 years ago

Goal here is to add a new signal protocol to benchmark-wrapper. Benchmark-wrapper is currently missing the ability to export state, making collecting metrics using external tools unnecessarily difficult and inaccurate. In this enhancement, we want to allow for benchmark-wrapper to export the state of the benchmark run to redis.

Additions:

webbnh commented 2 years ago

Create functionality that allows for benchmarks to understand how many consumers have acknowledged a published signal.

I think this is a mis-specification: I think the requirement is for the SignalExporter class to be able to synchronize the signal export with the subscribers. In order to do this, the class will, among other things, need to know how many subscribers there are (so that it can determine when they've all acknowledged the signal), but this is an implementation detail, not an architectural requirement (synchronization could, for instance, be implemented in other ways).

    ---o--O--o---

The first item needed is an architecture and protocol description laying out how benchmark runners like Benchmark-wrapper can publish event notifications to subscribers like Pbench. (There should be no expectation that Benchmark-wrapper will be the only runner to use this architecture, and there should be no expectation that Pbench will be the only subscriber.)

Items which need to be outlined include:

And then a more functional set of descriptions is needed:

learnitall commented 2 years ago

I think this is a mis-specification: I think the requirement is for the SignalExporter class to be able to synchronize the signal export with the subscribers.

s/Create functionality that allows for benchmarks to understand how many consumers have acknowledged a published signal./Create functionality within the SignalExporter class which benchmarks can use to understand if consumers are ready for the benchmark to begin./

There should be no expectation that Benchmark-wrapper will be the only runner to use this architecture

Why is this? I understand this:

there should be no expectation that Pbench will be the only subscriber.

But the only use case that the cloud-bulldozer org has for this protocol is for exporting state from benchmark-wrapper (correct me if I'm wrong here @jtaleric @rsevilla87). If there are going to be other tools which use this protocol then shouldn't it live outside of benchmark-wrapper? Otherwise hosting it within benchmark-wrapper seems arbitrary. I don't think it really matters anyways either way, but just curious as to why we have this expectation when we've only talked about the protocol within the scope of benchmark-wrapper.

I think your included architecture requirements and functional description requirements are spot on, I'll get to work on filling those out.

learnitall commented 2 years ago

I've been super caught up in https://github.com/cloud-bulldozer/benchmark-wrapper/pull/299, as well some other changes for migrating benchmark-wrapper over to the more object-orientated structure. @maxusmusti do you want to get started on the design of the protocol and we can touch base on implementation?

Maxusmusti commented 2 years ago

I've been super caught up in #299, as well some other changes for migrating benchmark-wrapper over to the more object-orientated structure. @Maxusmusti do you want to get started on the design of the protocol and we can touch base on implementation?

Working on design doc here