Use metrics instead of stats in SD exporter#560
Use metrics instead of stats in SD exporter#560c24t merged 16 commits intocensus-instrumentation:stackdriver-metricsfrom
Conversation
|
|
||
|
|
||
| class StackdriverStatsExporter(base_exporter.StatsExporter): | ||
| class StackdriverStatsExporter(object): |
There was a problem hiding this comment.
We'll update StatsExporter after making the same changes to the prometheus exporter. This is an API change.
| except KeyError: | ||
| raise ValueError("Unknown MetricDescriptorType value") | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Consider replacing this one-off enum class with enum34.
| def create_time_series_list(self, metric): | ||
| tsl = [] | ||
| for ts in metric.time_series: | ||
| tsl.append(self._convert_series(metric, ts)) |
There was a problem hiding this comment.
Curious, would this [self._convert_series(metric, ts) for ts in metric.time_series] give better readability and perf? And would tuple work here?
There was a problem hiding this comment.
Changed in bce0b86. No strong opinion on lists vs tuples, but it's fine for the caller to modify this one and we generally use lists internally like this.
| """Get a SD descriptor type for an OC metric descriptor.""" | ||
| return namespaced_view_name(oc_md.name, self.options.metric_prefix) | ||
|
|
||
| def get_metric_descriptor(self, oc_md): |
There was a problem hiding this comment.
Minor suggestion, probably we can use a predefined dict to do the lookup?
There was a problem hiding this comment.
agreed. In fact one already exists for gauge: metric_utils.is_gauge. I think it would make sense to create one for cumulative and for the value types as well.
| safe_key = sanitize_label(key.key) | ||
| series.metric.labels[safe_key] = val.value | ||
|
|
||
| set_monitored_resource(series, self.options.resource) |
There was a problem hiding this comment.
Outside the scope of this pr, but I'm curious what the motivation is to do the monitored resource detection every time we export every series. Instead would it make sense to detect and convert the labels once when the exporter is created? Then we can just set it on each series each time.
| """Get a SD descriptor type for an OC metric descriptor.""" | ||
| return namespaced_view_name(oc_md.name, self.options.metric_prefix) | ||
|
|
||
| def get_metric_descriptor(self, oc_md): |
There was a problem hiding this comment.
agreed. In fact one already exists for gauge: metric_utils.is_gauge. I think it would make sense to create one for cumulative and for the value types as well.
c24t
changed the title
|
e89c84e is a mostly mechanical change to the existing tests to convert Note that this is being merged into the |
| GLOBAL_RESOURCE_TYPE = 'global' | ||
|
|
||
| # OC metric descriptor type to SD metric kind and value type | ||
| OC_MD_TO_SD_TYPE = { |
There was a problem hiding this comment.
There should be one more mapping for Summary type. See Java: census-instrumentation/opencensus-java#1591. You can file an issue to add support for it later.
contrib/opencensus-ext-stackdriver/opencensus/ext/stackdriver/stats_exporter/__init__.py
Show resolved
Hide resolved
| elif (metric.descriptor.type == | ||
| metric_descriptor.MetricDescriptorType.GAUGE_DOUBLE): | ||
| sd_point.value.double_value = float(point.value.value) | ||
|
|
There was a problem hiding this comment.
elif (metric.descriptor.type ==
metric_descriptor.MetricDescriptorType.SUMMARY):
# TODO...
|
Our stackdriver system tests create (and then don't delete) metric descriptors in GCP. |
5 participants
This is the first of several breaking changes to replace the stats data model (
ViewData,AggregationData, etc.) with metrics in stats/metrics exporters.This PR changes
StackdriverStatsExporterto:Metricinstead ofViewDatainternallyon_register_viewhook, lazily register metric descriptors with stackdriver at export time instead of on view creationemit, etc.The meat of this PR is in
create_time_series_list, which now takes a list ofMetrics.cc @colincadams
Addresses #454.