Changed summaries to histograms in DecisionEngine and TaskManager mod…

[shreyb]

…ules

Upon discussion with K. Retzke, we decided that it's probably more useful to use histograms for general timed metrics, since Summaries only give us size and number of events, whereas Histograms do the same and bucket the information automatically.

[codecov[bot]]

Codecov Report

Merging #569 (37385d5) into master (6fa0bf4) will decrease coverage by 14.02%. The diff coverage is 85.71%.

@@             Coverage Diff             @@
##           master     #569       +/-   ##
===========================================
- Coverage   95.39%   81.36%   -14.03%     
===========================================
  Files          45       45               
  Lines        2716     2716               
  Branches      442      495       +53     
===========================================
- Hits         2591     2210      -381     
- Misses         91      468      +377     
- Partials       34       38        +4     

Flag	Coverage Δ
python-3.10	81.29% <85.71%> (-13.96%)	:arrow_down:
python-3.6	80.91% <85.71%> (-14.17%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../decisionengine/framework/engine/DecisionEngine.py	32.89% <76.92%> (-54.04%)	:arrow_down:
...ecisionengine/framework/taskmanager/TaskManager.py	86.68% <100.00%> (-11.69%)	:arrow_down:
...rc/decisionengine/framework/modules/SourceProxy.py	26.08% <0.00%> (-52.18%)	:arrow_down:
...cisionengine/framework/taskmanager/module_graph.py	64.15% <0.00%> (-35.85%)	:arrow_down:
src/decisionengine/framework/engine/Workers.py	69.44% <0.00%> (-18.06%)	:arrow_down:
src/decisionengine/framework/dataspace/maintain.py	82.85% <0.00%> (-17.15%)	:arrow_down:
...isionengine/framework/managers/ComponentManager.py	82.92% <0.00%> (-9.76%)	:arrow_down:
src/decisionengine/framework/modules/Module.py	91.89% <0.00%> (-8.11%)	:arrow_down:
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6fa0bf4...37385d5. Read the comment docs.

[retzkek]

Note the following:

The default buckets are intended to cover a typical web/rpc request from milliseconds to seconds. They can be overridden by passing buckets keyword argument to Histogram.

https://github.com/prometheus/client_python#histogram

A little thought should be given to decide if the defaults are reasonable for these metrics, and if you want to make it configurable.

    DEFAULT_BUCKETS = (.005, .01, .025, .05, .075, .1, .25, .5, .75, 1.0, 2.5, 5.0, 7.5, 10.0, INF)

https://github.com/prometheus/client_python/blob/master/prometheus_client/metrics.py#L526

[shreyb]

Note the following:

The default buckets are intended to cover a typical web/rpc request from milliseconds to seconds. They can be overridden by passing buckets keyword argument to Histogram.

https://github.com/prometheus/client_python#histogram

A little thought should be given to decide if the defaults are reasonable for these metrics, and if you want to make it configurable.

    DEFAULT_BUCKETS = (.005, .01, .025, .05, .075, .1, .25, .5, .75, 1.0, 2.5, 5.0, 7.5, 10.0, INF)

https://github.com/prometheus/client_python/blob/master/prometheus_client/metrics.py#L526

That's a good point. Given that it's already configurable by invocation, I don't know if we want to add "clutter" to the jsonnet file. But I'll make a note in the developer documentation that buckets can be passed in.

[retzkek]

Given that it's already configurable by invocation, I don't know if we want to add "clutter" to the jsonnet file. But I'll make a note in the developer documentation that buckets can be passed in.

I agree, if the buckets do need to be changed probably best just to do it in code.