Update System Metrics#1019
Conversation
ocelotl
force-pushed
the
issue_1006
branch
3 times, most recently
from
dac057f to
93f6df5
Compare
ocelotl
force-pushed
the
issue_1006
branch
2 times, most recently
from
3295f60 to
ffae05a
Compare
...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py
Outdated
Show resolved
Hide resolved
| description="System CPU utilization", | ||
| unit="1", | ||
| value_type=float, | ||
| observer_type=UpDownSumObserver, |
There was a problem hiding this comment.
My bad @ocelotl, in OTEP 119 I think all of the UpDownSumObserver should be ValueObserver (see open-telemetry/opentelemetry-specification#819).
The metrics API points out the similarity/confusion between UpDownSumObserver and ValueObserver with the example of measuring queue size:
Therefore, considering the choice between ValueObserver and UpDownSumObserver, the recommendation is to choose the instrument with the more-appropriate default aggregation. If you are observing a queue size across a group of machines and the only thing you want to know is the aggregate queue size, use SumObserver because it produces a sum, not a distribution. If you are observing a queue size across a group of machines and you are interested in knowing the distribution of queue sizes across those machines, use ValueObserver.
Commented for tracking/discussion in a spec issue open-telemetry/opentelemetry-specification#818 (comment)
There was a problem hiding this comment.
Ok, changed them. PTAL and let me know it this is what you are expecting 👍
...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py
Outdated
Show resolved
Hide resolved
aabmass
left a comment
aabmass
left a comment
There was a problem hiding this comment.
Looks like Bogdan and Josh are advocating towards the .utilization metrics being ValueObserver and the others ones staying as UpDownSumObserver. We can just update once the spec comes out though. LGTM!
...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py
Outdated
Show resolved
Hide resolved
...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py
Outdated
Show resolved
Hide resolved
...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py
Outdated
Show resolved
Hide resolved
Ok, do you think we should keep the observer as it is right now for network connections? |
|
Maybe change that one to UpDownSumObserver, then this LGTM! |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Just one question, otherwise this looks great
...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py
Outdated
Show resolved
Hide resolved
codeboten
added
the
instrumentation
...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py
Outdated
Show resolved
Hide resolved
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
One more update needed for the documentation
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Thanks for the update!
3 participants
Update System Metrics as per open-telemetry/oteps#119