Prometheus metric exporter #378

hectorhdzg · 2020-01-24T22:07:08Z

This implementation is based on OpenCensus Prometheus exporter

I added a lot of code from #341 to unblock testing and lint for this, once Metrics code is updated this PR only need to merge with latest changes. Please review Prometheus related changes and add comments in PR 341 PR for Metric related stuff.

There are still TODOs regarding supporting histograms and adding tests for Gauges and Measures, this is currently blocked by missing Aggregators in opentelemetry SDK

Fixes #157

…heus_exporter

codecov-io · 2020-01-25T01:21:55Z

Codecov Report

Merging #378 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
+ Coverage   88.11%   88.25%   +0.14%     
==========================================
  Files          41       41              
  Lines        2078     2078              
  Branches      238      238              
==========================================
+ Hits         1831     1834       +3     
+ Misses        173      172       -1     
+ Partials       74       72       -2

Impacted Files	Coverage Δ
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`91.2% <100%> (+0.36%)`	⬆️
...elemetry-api/src/opentelemetry/context/__init__.py	`67.39% <0%> (+4.34%)`	⬆️

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 35b19b3...bfd815b. Read the comment docs.

ocelotl

Just a few comments/questions.

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

opentelemetry-api/src/opentelemetry/metrics/__init__.py

mauriciovasquezbernal

Overall the PR looks good, I have a question about the controller type that is used.

This PR uses a PushController, it periodically collects the metrics and exports them. The exporter saves those metrics until it is pulled from a client (through the http endpoint that is exposed by start_http_server). I'm wondering if that fact that the same metric could be shown multiple times when the endpoint is called (the controller made multiple iterations), or could not be shown at all (the controller didn't make any iteration) is a problem.

I feel this kind of exporter fits much better with a PullController, would it make sense to implement it before this PR?

ext/opentelemetry-ext-prometheus/README.rst

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

ext/opentelemetry-ext-prometheus/README.rst

…heus_exporter

hectorhdzg · 2020-02-11T23:54:23Z

@mauriciovasquezbernal @ocelotl I tried to address all comments please let me know if you have more feedback, we can update examples to use new PullController when available

mauriciovasquezbernal

Added some more comments, more of them are just nits. Only one really important about the structure used to pass store metrics records.

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

ext/opentelemetry-ext-prometheus/README.rst

ext/opentelemetry-ext-prometheus/tests/__init__.py

ext/opentelemetry-ext-prometheus/tests/test_prometheus_exporter.py

docs/opentelemetry.metrics.rst

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

ocelotl · 2020-02-12T17:05:05Z

ext/opentelemetry-ext-prometheus/tests/test_prometheus_exporter.py

+
+    def test_gauge_to_prometheus(self):
+        # TODO: Add unit test once GaugeAggregator is available
+        self.assertEqual(True, True)


Even better, remove the test function and just leave the TODO comment. Pytest still collects, runs and reports this test as it was meaningful when it is not.

ocelotl · 2020-02-12T17:05:22Z

ext/opentelemetry-ext-prometheus/tests/test_prometheus_exporter.py

+
+    def test_measure_to_prometheus(self):
+        # TODO: Add unit test once Measure Aggregators are available
+        self.assertEqual(True, True)


ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

mauriciovasquezbernal

I tried the example and it didn't work for me. Besides that LGTM!.

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

hectorhdzg · 2020-02-13T02:05:42Z

@mauriciovasquezbernal are you getting any error when running the example?, did you run pip install -e .\opentelemetry-ext-prometheus?

mauriciovasquezbernal · 2020-02-13T02:12:50Z

@hectorhdzg I'm sorry for not being clear. It was not working for me because the check_point vs checkpoint, now that you have renamed it, it should be fine.

mauriciovasquezbernal

I'm approving it with the PushController because we don't have defined an architecture for the PullController yet. Probably we want to iterate over this implementation once we have the PullController implemented.

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

lzchen · 2020-02-14T18:24:26Z

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

+    def collect(self):
+        """Collect fetches the metrics from OpenTelemetry
+        and delivers them as Prometheus Metrics.
+        Collect is invoked every time a prometheus.Gatherer is run


Is there a link to some API that shows how this works?

The custom collector will use same logic as regular Prometheus Collector, pulling mechanism is configured by time intervals but is also triggered by other input like the user refreshing Prometheus visualization UI

https://github.com/prometheus/client_python#custom-collectors
https://github.com/prometheus/client_python/blob/master/prometheus_client/registry.py

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

…heus_exporter

lzchen · 2020-02-14T19:04:10Z

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

+        for label_key in metric_record.metric.label_keys:
+            label_keys.append(self._sanitize(label_key))
+        metric_name = ""
+        if self._prefix != "":


You can do if self._prefix: I believe

lzchen · 2020-02-14T19:08:23Z

Looks good! Just some questions about how the label keys and label values behavior is handled.

examples/metrics/prometheus.py

lzchen

lgtm

hectorhdzg · 2020-02-18T23:33:55Z

@ocelotl let me know if you have more feedback for this one

ocelotl

LGTM :+1+

toumorokoshi

I'm sorry to come in so late into a PR, especially one that's ready to merge. But I think the design on how prometheus metrics are exposed requires at the minimum a conversation.

I think we should really split out the starting of the prometheus_client's exposure of metrics from the linking to a Meter object. The current pattern of always choosing to use prometheus_client.start_http_server makes some really valuable use cases very hard to accomplish:

I already have a prometheus http server / wsgi server started, and I want to add opentelemetry metrics to it.
I cannot choose to mount my prometheus route into my wsgi / asgi app
cannot support any future run modes of prometheus_client.

Why not just require that someone reads through prometheus_client, sets up their prometheus endpoint, then attaches via PushController a PrometheusExporter to Meter? It doesn't add that many lines of code, and separates the route startup from the adding of metrics to a registry.

Aside from that, one other question:

Performance of Prometheus in multi-process environments?

I'm curious for those who are familiar with OpenCensus: what was the popularity of the prometheus integration? For me, I'm not sure if it'll handle high throughput applications with it's use of a file to perform IPC around metrics:

There is also a general weakness in the prometheus client where it uses a file to perform IPC, and some features (such as registries) do not behave like normal:

https://github.com/prometheus/client_python#multiprocess-mode-gunicorn

https://github.com/prometheus/client_python/blob/master/prometheus_client/multiprocess.py

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py

…heus_exporter

hectorhdzg · 2020-02-20T23:23:42Z

I removed start_http server call from exporter, users can use any kind of Prometheus client and we just attach our Collector to the Prometheus registry, added extra call in README and sample so people can see their data when testing the exporter. @toumorokoshi, @codeboten , @lzchen and @ocelotl please let me know if this ok for everyone.

toumorokoshi

Thanks! looks good. I'll leave it open for a little bit longer in case anyone wants to comment on the new pattern, but otherwise we'll merge tomorrow.

…#378)

hectorhdzg added 6 commits January 17, 2020 13:10

WIP

9850fcc

WIP

5d83631

WIP

20c190a

Merge remote-tracking branch 'upstream/master' into hectorhdzg/promet…

a4692e1

…heus_exporter

Prometheus exporter

8493acd

Updating readme

cb58432

hectorhdzg requested review from a team, c24t and lzchen January 24, 2020 22:07

hectorhdzg added 4 commits January 24, 2020 14:45

Updating export tests

8f4f968

Fixing API tests

f359148

Fixing docs

8299efc

Fixing docs

4aa9d4d

ocelotl reviewed Jan 29, 2020

View reviewed changes

mauriciovasquezbernal reviewed Feb 10, 2020

View reviewed changes

ext/opentelemetry-ext-prometheus/README.rst Outdated Show resolved Hide resolved

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py Outdated Show resolved Hide resolved

ext/opentelemetry-ext-prometheus/README.rst Show resolved Hide resolved

hectorhdzg added 3 commits February 11, 2020 13:09

Merge remote-tracking branch 'upstream/master' into hectorhdzg/promet…

e5f2cf1

…heus_exporter

Updating to latest metrics code

0d3e4b4

Addressing comments

19489bf

hectorhdzg added 2 commits February 11, 2020 16:38

Updating to latest metrics raw

4db034a

Update

3c2fc3a

mauriciovasquezbernal reviewed Feb 12, 2020

View reviewed changes

ocelotl suggested changes Feb 12, 2020

View reviewed changes

Addressing comments

509cf96

mauriciovasquezbernal suggested changes Feb 13, 2020

View reviewed changes

Updating checkpoint property

35f975d

mauriciovasquezbernal approved these changes Feb 13, 2020

View reviewed changes

lzchen reviewed Feb 14, 2020

View reviewed changes

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Feb 14, 2020

View reviewed changes

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Feb 14, 2020

View reviewed changes

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py Show resolved Hide resolved

lzchen reviewed Feb 14, 2020

View reviewed changes

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into hectorhdzg/promet…

aaa5448

…heus_exporter

lzchen reviewed Feb 14, 2020

View reviewed changes

Addressing comments

3023ff1

mauriciovasquezbernal reviewed Feb 14, 2020

View reviewed changes

examples/metrics/prometheus.py Show resolved Hide resolved

Updating tests

37ea8ee

lzchen approved these changes Feb 14, 2020

View reviewed changes

lzchen mentioned this pull request Feb 19, 2020

Metric exporters to prepend name of meter to metric instruments #432

Closed

ocelotl approved these changes Feb 19, 2020

View reviewed changes

hectorhdzg added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 19, 2020

toumorokoshi suggested changes Feb 20, 2020

View reviewed changes

ext/opentelemetry-ext-prometheus/src/opentelemetry/ext/prometheus/__init__.py Outdated Show resolved Hide resolved

toumorokoshi removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 20, 2020

hectorhdzg added 2 commits February 20, 2020 15:18

Removing start_server

77178b8

Merge remote-tracking branch 'upstream/master' into hectorhdzg/promet…

35b19b3

…heus_exporter

hectorhdzg and others added 2 commits February 20, 2020 15:24

Merge branch 'master' into hectorhdzg/prometheus_exporter

555bc36

Fix lint

bfd815b

toumorokoshi approved these changes Feb 21, 2020

View reviewed changes

toumorokoshi added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 21, 2020

toumorokoshi merged commit 26e0576 into open-telemetry:master Feb 21, 2020

c24t removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Feb 21, 2020

Bhargavamacha mentioned this pull request Mar 2, 2020

Create a PullController for metrics open-telemetry/opentelemetry-js#796

Closed

hectorhdzg deleted the hectorhdzg/prometheus_exporter branch March 24, 2020 22:35

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

fix(jaeger): assign correct scope for flush operation (open-telemetry…

b0f3d3e

…#378)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus metric exporter #378

Prometheus metric exporter #378

hectorhdzg commented Jan 24, 2020 •

edited

Loading

codecov-io commented Jan 25, 2020 •

edited

Loading

ocelotl left a comment

mauriciovasquezbernal left a comment

hectorhdzg commented Feb 11, 2020

mauriciovasquezbernal left a comment

ocelotl Feb 12, 2020

ocelotl Feb 12, 2020

mauriciovasquezbernal left a comment

hectorhdzg commented Feb 13, 2020

mauriciovasquezbernal commented Feb 13, 2020

mauriciovasquezbernal left a comment

lzchen Feb 14, 2020

hectorhdzg Feb 14, 2020

lzchen Feb 14, 2020

lzchen commented Feb 14, 2020

lzchen left a comment

hectorhdzg commented Feb 18, 2020

ocelotl left a comment

toumorokoshi left a comment

hectorhdzg commented Feb 20, 2020

toumorokoshi left a comment

Prometheus metric exporter #378

Prometheus metric exporter #378

Conversation

hectorhdzg commented Jan 24, 2020 • edited Loading

codecov-io commented Jan 25, 2020 • edited Loading

Codecov Report

ocelotl left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

hectorhdzg commented Feb 11, 2020

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

ocelotl Feb 12, 2020

Choose a reason for hiding this comment

ocelotl Feb 12, 2020

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

hectorhdzg commented Feb 13, 2020

mauriciovasquezbernal commented Feb 13, 2020

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

lzchen Feb 14, 2020

Choose a reason for hiding this comment

hectorhdzg Feb 14, 2020

Choose a reason for hiding this comment

lzchen Feb 14, 2020

Choose a reason for hiding this comment

lzchen commented Feb 14, 2020

lzchen left a comment

Choose a reason for hiding this comment

hectorhdzg commented Feb 18, 2020

ocelotl left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Performance of Prometheus in multi-process environments?

hectorhdzg commented Feb 20, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

hectorhdzg commented Jan 24, 2020 •

edited

Loading

codecov-io commented Jan 25, 2020 •

edited

Loading