Prometheus exporter #294

vcasadei · 2018-09-04T01:07:15Z

No description provided.

Removing stats exporter stuff Removing unused imports Minor changes in comments and copyright Unit tests for LastValue aggregation

…tructure

Removing stats exporter stuff Removing unused imports Minor changes in comments and copyright Unit tests for LastValue aggregation

…ation#257)

…ion#258)

…tructure

…attributes (census-instrumentation#272)

* Fix bug in updating aggregation map. * Update unit tests. * Use tag_value_aggregation_data_map instead of tag_value_aggregation_map.

odeke-em · 2018-09-06T16:29:33Z

README.rst

@@ -516,3 +573,4 @@ Disclaimer
 ----------

 This is not an official Google product.
+gi


odeke-em · 2018-09-06T16:30:00Z

opencensus/stats/aggregation_data.py

@@ -230,7 +230,6 @@ def increment_bucket_count(self, value):
            return i

        self._counts_per_bucket[(len(self._bounds))-1] += 1
-


Spurious line? Please remove it.

liyanhui1228 · 2018-09-06T18:23:31Z

examples/stats/exporter/prometheus.py

+    measure_map.measure_int_put(VIDEO_SIZE_MEASURE, 25 * MiB)
+    measure_map.record(tag_map)
+
+    # Use the line below to see the data on prometheus


Remove the lines here?

Hmm, I'm not sure. If we remove, the user may not be able to access Prometheus before the code finishes. This is just a way to time.sleep() for an uncertain amount of time and allow the user to see the collected data on Prometheus. Do you have any suggestions on how to do that in a better way?

liyanhui1228 · 2018-09-06T18:26:20Z

opencensus/stats/exporters/prometheus_exporter.py

+        return self._registered_views
+
+    def register_view(self, view):
+        """ register_view will create the needed structure


Start with uppercase letter and same for the doc lines below.

It is lower case because we start with the method name (as done in other classes). Do you prefer if we start with the description, like Will create...?

liyanhui1228 · 2018-09-06T18:28:25Z

opencensus/stats/exporters/prometheus_exporter.py

+        signature = view_signature(self.options.namespace, view_data.view)
+        self.view_data[signature] = view_data
+
+    def to_metric(self, desc, view):


Add docs for args and returns here.

Done! Please let me know if it can be improved.

liyanhui1228 · 2018-09-06T18:30:15Z

opencensus/stats/exporters/prometheus_exporter.py

+        agg_data = view.aggregation.aggregation_data
+
+        if isinstance(agg_data, aggregation_data_module.CountAggregationData):
+            labels = desc['labels'] if agg_data.count_data is None else None


This looks a little strange, could you explain the relationship between desc labels and agg_data.count_data?

desc['labels'] has the Aggregation Data columns. So, if there is no aggregation data to get the columns from, it gets from desc. This may be necessary at the begging of an aggregation, when no data point was recorded.

liyanhui1228 · 2018-09-11T17:10:03Z

opencensus/stats/exporters/prometheus_exporter.py

+class PrometheusStatsExporter(base.StatsExporter):
+    """ Exporter exports stats to Prometheus, users need
+        to register the exporter as an HTTP Handler to be
+        able to export.


Add docstrings about the params uses in the constructor.

Done! Please let me know if it can be improved.

liyanhui1228 · 2018-09-11T17:11:17Z

opencensus/stats/exporters/prometheus_exporter.py

+                 gatherer,
+                 transport=sync.SyncTransport,
+                 collector=Collector()):
+        self._options = options


Looking at the Options class, all of its args have default value and are optional. So can we also make the options param here optional?

if options is None: options = Options()

Indeed it has default values, but at least the namespace has to be changed since it is an empty string and new_stats_exporter() doesn't allow empty namespaces in the options. So, unless we set a non-empty namespace as default or allow empty namespaces to create exporters, we have to keep this way.

odeke-em · 2018-09-11T17:57:25Z

@vcasadei thanks for the PR! However, how come this PR has more than 20 commits that are unrelated to this PR? Could you please properly rebase it with master so that it is up to-date but also
so that we have the commits relevant to this PR?

odeke-em · 2018-09-11T18:18:24Z

examples/stats/exporter/prometheus.py

+import time
+
+from opencensus.stats import aggregation as aggregation_module
+from opencensus.stats.exporters import prometheus_exporter as prometheus


Why don't we keep the file name as prometheus? Instead of prometheus_exporter, which already resides under opencensus/stats/exporters, hence the _exporter suffix here there is redundant.

I got your point, but this is how the exporters files where named in trace and stats before. I believe they were just following the pattern. But I like your suggestion, I think we can open an issue about this.

odeke-em · 2018-09-11T18:19:23Z

opencensus/stats/exporters/prometheus_exporter.py

@@ -0,0 +1,339 @@
+# Copyright 2018, OpenCensus Authors


I commented about keeping this file name as prometheus.py instead of prometheus_exporter.py because it already resides under opencensus/stats/exporters so the _exporter.py suffix is redundant.

Same of the last discussion 😄

odeke-em · 2018-09-11T18:20:16Z

opencensus/stats/exporters/prometheus_exporter.py

+    """
+    def __init__(self,
+                 namespace='',
+                 port=8000,


Let's document this default port.

Done, thanks for noticing. Please let me know if it can be improved.

eduardoemery · 2018-09-17T18:20:48Z

Hey guys. I'll give a little help here.

googlebot · 2018-09-19T16:53:58Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

MarceloAquino7 and others added 30 commits August 14, 2018 14:41

Squashing commits

f839e21

Removing stats exporter stuff Removing unused imports Minor changes in comments and copyright Unit tests for LastValue aggregation

Few adjustments due to Emmanuel's review

f89b60a

Fix due to Angela's reply | Created unit test for the new transport s…

ed6f363

…tructure

Renaming file

4ec6dc3

Sync transport unit test

15f687f

Lint fixes

9397a8a

Lint fixes

750fd30

Merge branch 'master' into stats_core_changes

1edfeba

Add tests for situations with exporter registered

123fc3b

Merge branch 'master' into stats_core_changes

ecaacc3

Bringing content from another branch

ec8cbf3

Some fixes due to code review

6ce4afb

support W3C distributed tracing (part 1) (census-instrumentation#251)

a14667e

Fix typos in the README. (census-instrumentation#252)

033a1ea

Squashing commits

3722f70

Removing stats exporter stuff Removing unused imports Minor changes in comments and copyright Unit tests for LastValue aggregation

Tags: Set and get current tag_map in thread local. (census-instrument…

a492f88

…ation#257)

add tracestate to spancontext (census-instrumentation#259)

8a9417e

Stats: Allow recording against implicit tag_map. (census-instrumentat…

ff0b9a0

…ion#258)

Few adjustments due to Emmanuel's review

eb8ba4f

support tracestate HTTP propagation (census-instrumentation#261)

05f653c

Fix census-instrumentation#263 (census-instrumentation#264)

5c7460f

Prepare for 0.1.6 release (census-instrumentation#267)

9515063

Fix due to Angela's reply | Created unit test for the new transport s…

3827a23

…tructure

Renaming file

4fb0061

Sync transport unit test

3fde245

Lint fixes

3fd06bd

Lint fixes

338f173

Set SpanKind in extensions (census-instrumentation#271)

1d1d4c8

Rename attributes to the default naming schema and use standard http …

549a5e1

…attributes (census-instrumentation#272)

Fix bug in updating aggregation map. (census-instrumentation#253)

ad877b0

* Fix bug in updating aggregation map. * Update unit tests. * Use tag_value_aggregation_data_map instead of tag_value_aggregation_map.

vcasadei added 3 commits September 6, 2018 12:15

Fixing readme

52067bd

cover

92fb0cc

Lint

006f94b

odeke-em reviewed Sep 6, 2018

View reviewed changes

vcasadei and others added 3 commits September 6, 2018 13:53

Fix readme

5bef7f0

lint

f442bc5

Merge branch 'master' into prometheus_exporter

a905d2b

liyanhui1228 requested changes Sep 11, 2018

View reviewed changes

odeke-em reviewed Sep 11, 2018

View reviewed changes

eduardoemery force-pushed the prometheus_exporter branch 5 times, most recently from e0b97ee to 7706ed7 Compare September 17, 2018 19:32

Addresses review comments

ed73ffd

eduardoemery force-pushed the prometheus_exporter branch from 7706ed7 to ed73ffd Compare September 17, 2018 19:52

liyanhui1228 approved these changes Sep 19, 2018

View reviewed changes

Merge branch 'master' into prometheus_exporter

191738b

songy23 added cla: yes and removed cla: no labels Sep 19, 2018

liyanhui1228 merged commit 0e586b3 into census-instrumentation:master Sep 19, 2018

odeke-em mentioned this pull request Sep 22, 2018

quickstart/python: complete stats tutorial census-instrumentation/opencensus-website#354

Closed

c24t mentioned this pull request Oct 8, 2018

Make a few files non-executable #341

Merged

songy23 mentioned this pull request Oct 17, 2018

stats/prometheus: running the example fails to send data to the Prometheus UI #349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus exporter #294

Prometheus exporter #294

vcasadei commented Sep 4, 2018

odeke-em Sep 6, 2018

vcasadei Sep 6, 2018

odeke-em Sep 6, 2018

liyanhui1228 Sep 6, 2018

eduardoemery Sep 17, 2018

liyanhui1228 Sep 6, 2018

eduardoemery Sep 17, 2018

liyanhui1228 Sep 6, 2018

eduardoemery Sep 17, 2018

liyanhui1228 Sep 6, 2018

eduardoemery Sep 17, 2018

liyanhui1228 Sep 11, 2018

eduardoemery Sep 17, 2018

liyanhui1228 Sep 11, 2018

eduardoemery Sep 17, 2018 •

edited

Loading

odeke-em commented Sep 11, 2018

odeke-em Sep 11, 2018

eduardoemery Sep 17, 2018

odeke-em Sep 11, 2018

eduardoemery Sep 17, 2018

odeke-em Sep 11, 2018

eduardoemery Sep 17, 2018

eduardoemery commented Sep 17, 2018

googlebot commented Sep 19, 2018

		@@ -230,7 +230,6 @@ def increment_bucket_count(self, value):
		return i

		self._counts_per_bucket[(len(self._bounds))-1] += 1

Prometheus exporter #294

Prometheus exporter #294

Conversation

vcasadei commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardoemery Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

odeke-em commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardoemery commented Sep 17, 2018

googlebot commented Sep 19, 2018

eduardoemery Sep 17, 2018 •

edited

Loading