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

Access ExecuteResult or Measurements from a Diagnoser #2617

Open
NinoFloris opened this issue Aug 15, 2024 · 4 comments
Open

Access ExecuteResult or Measurements from a Diagnoser #2617

NinoFloris opened this issue Aug 15, 2024 · 4 comments

Comments

@NinoFloris
Copy link

NinoFloris commented Aug 15, 2024

I was trying to implement a cpu diagnoser (i.e. #1666) however I ran into issues doing so as DiagnoserResults only exposes TotalOperations.

Taking a look at the gist: https://gist.github.com/MarkPflug/55173728458020c6d335cc099c891c0b

Specifically https://gist.github.com/MarkPflug/55173728458020c6d335cc099c891c0b#file-cpudiagnoser-cs-L54-L66

public void Handle(HostSignal signal, DiagnoserActionParameters parameters)
{
    if(signal == HostSignal.BeforeActualRun)
    {
        userStart = proc.UserProcessorTime.Ticks;
        privStart = proc.PrivilegedProcessorTime.Ticks;
    }
    if(signal == HostSignal.AfterActualRun)
    {
        userEnd = proc.UserProcessorTime.Ticks;
        privEnd = proc.PrivilegedProcessorTime.Ticks;
    }
}

You can see it's measuring from before actual until the end of those runs (a contiguous measurement is needed).

This then gets emitted as metrics like so:

public IEnumerable<Metric> ProcessResults(DiagnoserResults results)
{
    yield return new Metric(CpuUserMetricDescriptor.Instance, 
        (userEnd - userStart) * 100d / results.TotalOperations);
    yield return new Metric(CpuPrivilegedMetricDescriptor.Instance, 
        (privEnd - privStart) * 100d / results.TotalOperations);
}

However as we don't know how many invocations and iterations it comprises of we cannot scale our stats correctly. Taking DiagnoserResults.TotalOperations is wrong here as it includes warmups and pilots, which we aren't measuring (and couldn't trivially measure in this way due to them being interleaved with other phases).

I'm a bit surprised to see the DiagnoserResults does not expose the actual run information from ExecuteResult - which it takes in its constructor. Doing so would allow people to come up with their own operation totals depending on their needs and measurement points.

Alternatively I would be helped with something like TotalWorkloadActualOperations but it feels like a slippery slope to add endless variations on data that should probably just be available in full fidelity.

@timcassell
Copy link
Collaborator

timcassell commented Aug 15, 2024

You could implement IExporter and get the results from the Summary.Reports and Report.AllMeasurements. You can also filter the measurements like .Where(m => m.Is(IterationMode.Workload, IterationStage.Actual).

[Edit] Although I'm not sure if you'll be able to add the results to the table, as I think the exporter runs after the diagnoser. Hm...

@timcassell
Copy link
Collaborator

Would it make sense to add the measurements to the DiagnoserResults @AndreyAkinshin @adamsitnik?

@NinoFloris
Copy link
Author

[Edit] Although I'm not sure if you'll be able to add the results to the table, as I think the exporter runs after the diagnoser. Hm...

Right, that's where I got stuck as well.

@adamsitnik
Copy link
Member

I'm a bit surprised to see the DiagnoserResults does not expose the actual run information from ExecuteResult -

It's not like we are against it, so far there was no need for it. @NinoFloris would you like to send a PR?

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

No branches or pull requests

3 participants