ext/system-metrics: adding instrumentation to collect system metrics by codeboten · Pull Request #652 · open-telemetry/opentelemetry-python

codeboten · 2020-05-05T17:53:28Z

Adding an extension to provide users an easy mechanism to collect metrics for their system. Some thoughts around this PR:

we could make which metrics are observed configurable
the semantic conventions around metric naming are still being defined so they are likely to change
the tests are currently checking the aggregators directly, I would like to change this to using the in-memory metrics exporter

The PR includes changes which I pulled out into this separate PR #653. I'll resolve if that one gets merged first.

Adding an extension to provide users an easy mechanism to collect metrics for their system. As part of this change, I also added an InMemoryMetricsExporter. I ended up adding meter_provider and memory_metrics_exporter to TestBase

mauriciovasquezbernal

Looks pretty good. Minor comments. I didn't review the pieces ported to #653.

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

ext/opentelemetry-ext-system-metrics/CHANGELOG.md

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

ext/opentelemetry-ext-system-metrics/tests/test_system_metrics.py

ext/opentelemetry-ext-system-metrics/README.rst

…tem_metrics/__init__.py Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

jmacd · 2020-05-07T17:17:41Z

we could make which metrics are observed configurable

I am hoping this becomes a general function of the SDK, through the Views API you should be able to disable or configure any instrument. Each plugin shouldn't expose a way to configure the instruments, ideally.

ocelotl

Looking good! Comments are non-blocking but worth taking a look 👍

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

ext/opentelemetry-ext-system-metrics/tests/test_system_metrics.py

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

lzchen · 2020-05-27T17:57:06Z

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

+            description="System memory",
+            unit="bytes",
+            value_type=int,
+            label_keys=self._labels.keys(),


I don't think you need to supply label_keys. Originally, these were served as "recommended keys", in which if the metric is recorded with labels that are not in label_keys, they would be dropped. Since you are adding the "type" label in each of the observes, they would theoretically be dropped. However, this functionality is going to be done in views (so label_keys here doesn't actually do anything). As well, from the specs we are most likely going to be getting rid of this field anyways.

lzchen · 2020-05-27T17:58:44Z

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

+            observer: the observer to update
+        """
+        system_memory = psutil.virtual_memory()
+        _metrics = [


I would define these outside of the callback. We don't want to keep instantiating these.

I think we should add an API to modify the _metrics for each system metric. This way, users that use this library will be able to customize the types of metrics for each system metric they export.

I agree, I've updated the constructor to support a config parameter to allow for other metrics to be configured. I think it can be improved, but maybe its enough to get started?

lzchen · 2020-05-27T17:59:30Z

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

+            "used",
+            "free",
+        ]
+        for metric in _metrics:


I would use "type" instead of "metrics".

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

…are about

codeboten · 2020-05-27T20:26:45Z

@lzchen addressed your comments if you get a chance to review

lzchen · 2020-05-29T17:32:39Z

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py

+        self.controller = PushController(
+            meter=self.meter, exporter=exporter, interval=interval
+        )
+        if config is None:


Is there somewhere that we can let the user know that passing in their own config is possible? Maybe an example file?

lzchen

LGTM. Just one comment about an example file.

…lemetry#652) * feat: port mongodb-core plugin to mongodb open-telemetry#622 * chore: address PR comments

codeboten added WIP Work in progress ext labels May 5, 2020

codeboten force-pushed the ext-system-metrics branch from f255ede to e99705e Compare May 6, 2020 15:49

codeboten marked this pull request as ready for review May 6, 2020 15:49

codeboten requested a review from a team May 6, 2020 15:49

codeboten removed the WIP Work in progress label May 6, 2020

codeboten changed the title ~~[WIP] ext/system-metrics: adding instrumentation to collect system metrics~~ ext/system-metrics: adding instrumentation to collect system metrics May 6, 2020

mauriciovasquezbernal reviewed May 6, 2020

View reviewed changes

Alex Boten and others added 3 commits May 6, 2020 14:47

fix lint and disable test for pypy

39af1c3

use labels passed into constructor + feedback from review

1bc093d

Update ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/sys…

ccc56d6

…tem_metrics/__init__.py Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label May 14, 2020

alrex added 4 commits May 19, 2020 12:10

Merge branch 'master' into ext-system-metrics

ddf2b9a

adding doc entry

e04a472

bump version number

e79f0e9

fix docs

35151b0

ocelotl approved these changes May 20, 2020

View reviewed changes

alrex and others added 6 commits May 21, 2020 15:26

Merge branch 'master' into ext-system-metrics

533136d

Merge branch 'master' into ext-system-metrics

84d3d9c

Apply suggestions from code review

048856f

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

move psutil requirement to install_requires

e7d6a25

test observer names

58b0e29

only report metrics available on all platforms

775baa9

lzchen reviewed May 27, 2020

View reviewed changes

refactor test to use in-memory exporter

0c4b5e4

lzchen reviewed May 27, 2020

View reviewed changes

ext/opentelemetry-ext-system-metrics/src/opentelemetry/ext/system_metrics/__init__.py Show resolved Hide resolved

Alex Boten added 4 commits May 27, 2020 11:47

support a config parameter to allow users to configure metrics they c…

b094635

…are about

adding runtime cpu

90565e5

fix lint and docs

fe751ea

Merge branch 'master' into ext-system-metrics

eb2410e

lzchen reviewed May 29, 2020

View reviewed changes

lzchen approved these changes May 29, 2020

View reviewed changes

alrex added 2 commits May 29, 2020 11:45

Merge branch 'master' into ext-system-metrics

d0e40f5

fixing version, adding configuration example

0460502

codeboten merged commit bfc54bf into open-telemetry:master May 29, 2020

lzchen mentioned this pull request Jun 1, 2020

Refactor out autocollection of metrics microsoft/opentelemetry-azure-monitor-python#94

Closed

Conversation

codeboten commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmacd commented May 7, 2020

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codeboten commented May 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codeboten commented May 5, 2020 •

edited

Loading