Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Switch SD exporter to polling#577

Merged
c24t merged 23 commits intocensus-instrumentation:stackdriver-metricsfrom
c24t:sdse-metrics-poll
Apr 1, 2019
Merged

Switch SD exporter to polling#577
c24t merged 23 commits intocensus-instrumentation:stackdriver-metricsfrom
c24t:sdse-metrics-poll

Conversation

@c24t
Copy link
Copy Markdown
Member

@c24t c24t commented Mar 25, 2019

This PR adds support for polling metrics from the stackdriver exporter. It builds on #560, which removed the ability to push metrics to the exporter via MeasureToViewMap.record.

I've added a PeriodicTask class (similar to the java client's IntervalMetricReader) that periodically calls exporter.export(producer.get_metrics()). To keep the API changes to a minimum I've changed new_stats_exporter to create and start the task, but we may want to change this to make the exporter ignorant of the transport instead.

@songy23 and @colincadams I'm interested to hear your thoughts on the API changes here.

@@ -0,0 +1,143 @@
# Copyright 2019, OpenCensus Authors
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note that this transport serves a different purpose than opencensus.common.transports.

On master:

exporter.export ->  exporter.transport.export -> exporter.emit -> ... -> exporter.create_time_series

vs. on this branch:

background_thread -> exporter.export -> exporter.create_time_series

I'm open to a different name or structure here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This serves a similar purpose to https://github.com/census-instrumentation/opencensus-go/blob/master/metric/metricexport/reader.go#L62, what about interval_metric_reader?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

High level question about this flow since I'm a little behind how stackdriver-metrics works. What happens when measure_to_view_map.export is called now? I see it is deprecated is it a no-op somehow?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nothing! We should be able to remove this and many other supporting classes (e.g. ViewData) once stackdriver and prometheus are migrated.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@songy23 it's long, but maybe worth it for the consistency between packages.

@c24t c24t force-pushed the sdse-metrics-poll branch from bd7c4d1 to 55190fa Compare March 25, 2019 23:31
@c24t c24t force-pushed the sdse-metrics-poll branch from fb7c44d to 0557df3 Compare March 26, 2019 01:22
@c24t c24t force-pushed the sdse-metrics-poll branch from 0557df3 to a6cb011 Compare March 26, 2019 01:29
Copy link
Copy Markdown
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Overall APIs look good, happy to see export in Python becomes more consistent with other languages.

@@ -0,0 +1,143 @@
# Copyright 2019, OpenCensus Authors
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This serves a similar purpose to https://github.com/census-instrumentation/opencensus-go/blob/master/metric/metricexport/reader.go#L62, what about interval_metric_reader?

Copy link
Copy Markdown
Contributor

@colincadams colincadams left a comment

Choose a reason for hiding this comment

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

Overall I like this! Would the goal be to eventually do this for all exporters, once stackdriver-metrics is complete and merged in?

@c24t
Copy link
Copy Markdown
Member Author

c24t commented Mar 26, 2019

Would the goal be to eventually do this for all exporters

All stats/metrics exporters at least. The idea is that metrics are generated by producers, and the whole stats module is a single producer. The API for exporting traces will still let us push.

@c24t c24t force-pushed the sdse-metrics-poll branch from 89e8a75 to c26f991 Compare March 29, 2019 02:03
@c24t c24t force-pushed the sdse-metrics-poll branch from 4a9207c to 50e1d4e Compare April 1, 2019 20:40
@c24t c24t merged commit 39cfba3 into census-instrumentation:stackdriver-metrics Apr 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants