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.

Sign up for GitHub

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

Jump to bottom

Update System Metrics #1019

Merged
merged 29 commits into from
Sep 4, 2020
Merged

Update System Metrics #1019

merged 29 commits into from
Sep 4, 2020
+1,146 −190

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Aug 19, 2020 •
edited
Loading

Update System Metrics as per open-telemetry/oteps#119

@ocelotl ocelotl self-assigned this Aug 19, 2020
@ocelotl ocelotl force-pushed the issue_1006 branch 3 times, most recently from dac057f to 93f6df5 Compare August 20, 2020 07:28
@ocelotl ocelotl changed the title WIP Update System Metrics Aug 20, 2020
@ocelotl ocelotl force-pushed the issue_1006 branch 2 times, most recently from 3295f60 to ffae05a Compare August 20, 2020 22:53
@ocelotl ocelotl marked this pull request as ready for review August 26, 2020 15:53
@ocelotl ocelotl requested a review from a team August 26, 2020 15:53
aabmass
aabmass reviewed Aug 26, 2020
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

...-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
description="System CPU utilization",
unit="1",
value_type=float,
observer_type=UpDownSumObserver,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ocelotl ocelotl requested a review from aabmass September 1, 2020 15:12
aabmass
aabmass approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ocelotl
Copy link
Contributor Author

ocelotl commented Sep 1, 2020

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!

Ok, do you think we should keep the observer as it is right now for network connections?

@aabmass
Copy link
Member

aabmass commented Sep 1, 2020

Maybe change that one to UpDownSumObserver, then this LGTM!

codeboten
codeboten reviewed Sep 3, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question, otherwise this looks great

...-instrumentation-system-metrics/src/opentelemetry/instrumentation/system_metrics/__init__.py Outdated Show resolved Hide resolved
@codeboten codeboten added the instrumentation Related to the instrumentation of third party libraries or frameworks label Sep 3, 2020
codeboten
codeboten reviewed Sep 3, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more update needed for the documentation

codeboten
codeboten approved these changes Sep 4, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

@codeboten codeboten merged commit 74ed13c into open-telemetry:master Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass approved these changes

@codeboten codeboten codeboten approved these changes

Assignees

@ocelotl ocelotl

Labels
instrumentation Related to the instrumentation of third party libraries or frameworks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ocelotl @aabmass @codeboten