Skeleton for Azure Metrics Exporter by lzchen · Pull Request #678 · census-instrumentation/opencensus-python

lzchen · 2019-06-06T21:35:40Z

Initial structure of the Azure Metrics Exporter.

Follows the StackDriver Stats Exporter logic. We use a poll-based mechanism instead of push-based as seen in the Prometheus Stats Exporter.

Currently, the exporter simply prints the meta data related to the metric.

@c24t @reyang

contrib/opencensus-ext-azure/README.rst

contrib/opencensus-ext-azure/opencensus/ext/azure/metric_exporter/__init__.py

reyang · 2019-06-06T23:49:39Z

contrib/opencensus-ext-azure/README.rst

+.. code:: python
+
+        from opencensus.ext.azure import metric_exporter
+        from opencensus.stats import stats as stats_module


Remove partial example code.

Move from stats to metrics, opencensus.stats is being replaced by opencensus.metrics. If we have dependency in stats, we need to refactor it and move to metrics.

contrib/opencensus-ext-azure/README.rst

contrib/opencensus-ext-azure/opencensus/ext/azure/metric_exporter/__init__.py

reyang · 2019-06-07T00:10:13Z

contrib/opencensus-ext-azure/opencensus/ext/azure/metric_exporter/__init__.py

+
+__all__ = ['MetricsExporter']
+
+class MetricsExporter(object):


Regarding the name, I wish to discuss about (we might want to do a better job in OpenTelemetry):

Do we want to drive for some level of consistency or not at all (and leave this for individual vendor's choice of flavor)?

When to use plural form versus not?

Should the class name be specific or generic? (e.g. do we want AzureExporter, AzureMonitorExporter, AzureMetricExporter or AzureMetricsExporter, my personal preference would be something we did for Azure trace exporter and stackdriver trace exporter)

Should the class name be self contained or we should leverage package name? (e.g. ext.azure.trace_exporter.AzureExporter, ext.azure.TraceExporter or ext.azure.AzureTraceExporter).

https://github.com/census-instrumentation/opencensus-python/blob/master/contrib/opencensus-ext-azure/opencensus/ext/azure/trace_exporter/__init__.py#L34

https://github.com/census-instrumentation/opencensus-python/blob/master/contrib/opencensus-ext-stackdriver/opencensus/ext/stackdriver/trace_exporter/__init__.py#L164

Yes I was thinking about this as well. I think it is good to be consistent across the entire project and set some standards.

In my opinion:

We should use singular form when referring to the "task-doer" such as MetricExporter, TraceExporter, etc. We should use the plural when we are referring to the category in a collective context (this will export metrics..., the traces are sent through..., etc) because the logic involves one or possibly more of these.

In regards to the class name, I think we should be as specific as possible. When going through the codebase, it is easier to identify what module I am looking at without having to look at the package I am under (problem that I am having with init.py). Also, if we ever take code snippets, readers can identify what class is being looked at without having to see the file path.

In regards to leveraging the package name, separating the files based off levels is my preference. So ext.azure.trace_exporter.AzureTraceExporter would be ideal.

@c24t what's your opinion?

Seems in OpenTelemetry Java we've already started to diverge (metrics and tags are plural form, while trace is not):

https://github.com/open-telemetry/opentelemetry-java/tree/master/api/src/main/java/io/opentelemetry

We have just discussed the exact same issue recently, in open-telemetry/opentelemetry-java#259 (comment). The conclusion is:

Let's stick with what we have, but pluralize resources.
trace
metrics
tags
resources

What about log? Using logs seems to be against language convention.

We haven't talked about it but personally I'd suggest to go with log for now. Agree logs doesn't seem to be a great name here.

I will make the changes based off these decisions for metrics.

lzchen · 2019-06-07T20:51:57Z

Going to refactor the PR to use metrics instead of stats.

…encensus-python into metrics

lzchen · 2019-06-10T23:07:05Z

The MetricsExporter uses copies of the metrics and view/recorder apis in stats (copied to metrics package). I know @c24t has been working on some refactor work from stats to metrics. Any thoughts on this?

#476

reyang · 2019-06-12T18:32:03Z

README.rst

 .. _threading: https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-threading
 .. _Zipkin: https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-zipkin

+Metrics Exporter


Probably need to put this under "Stats Exporter".

Yes I will do that. I will change it back to Metrics next time when we truly have a metrics exporter.

contrib/opencensus-ext-azure/README.rst

opencensus/metrics/export/metric_producer.py

contrib/opencensus-ext-azure/README.rst

reyang · 2019-06-13T02:06:41Z

contrib/opencensus-ext-azure/examples/logs/correlated.py

 import logging

-from opencensus.ext.azure.log_exporter import AzureLogHandler
+from opencensus.ext.azure.logs_exporter import AzureLogHandler


This is a breaking change, we need to:

Update the changelog to mention it, so users would know.

Bump the version when we do a release.

I would suggest that we don't touch this for now (since we haven't yet finalized whether to take plural form or not).

You're right. I did not realize this. I will revert this for now.

reyang

This PR is becoming too big.
Let's revert the log->logs change. Other parts looks good once we cleaned up CI error.

contrib/opencensus-ext-azure/README.rst

reyang · 2019-06-13T05:11:20Z

contrib/opencensus-ext-azure/README.rst

+Metrics
+~~~~~~~
+
+The **OpenCensus Azure Monitor Metrics Exporter** allows you to export metrics to `Azure Monitor`_.


Minor: I feel that we don't need to mention "OpenCensus" here given the context we have in the doc, otherwise it is too long?
Up to you, if you decide to keep it, need to update trace/logs to make them consistent.

Yes, you are right. We should be consistent with the other two and it seems a bit redundant given the package we are in. I shall remove it.

contrib/opencensus-ext-azure/README.rst

reyang · 2019-06-13T05:14:19Z

contrib/opencensus-ext-azure/README.rst

+        def main():
+            # Enable metrics
+            # Set the interval in seconds in which you want to send metrics
+            exporter = metrics_exporter.new_metrics_exporter(export_interval=2)


Minor suggestion, it is not clear if the interval should be an integer or it can be a float/decimal, consider either update the comment or put something like 2.0 (if we do support float).

We allow floats so I will put 2.0 in the sample to let users know implicitly this is possible.

Great, this is something that we've been practicing, e.g. ProbabilitySampler(1.0) and

aggregation_module.DistributionAggregation( [100.0, 200.0, 400.0, 1000.0, 2000.0, 4000.0]))

reyang · 2019-06-13T05:17:40Z

contrib/opencensus-ext-azure/opencensus/ext/azure/metrics_exporter/__init__.py

+
+
+def new_metrics_exporter(**options):
+    options = Options(**options)


I'm curious why would we want to validate options here versus in the MetricsExporter.__init__.

I feel like validation logic should be handled in the service layer, outside of the constructor. IMO init should only serve to instantiate variables.

Would you explain a bit more?
In general people do validate inside constructors, which prevent them from creating a wrong object.

https://github.com/googleapis/google-cloud-python/blob/master/monitoring/google/cloud/monitoring_v3/gapic/metric_service_client.py#L115

Hmm, it does seem like we do validation everywhere inside the constructor for a lot of classes. I can make the change to be consistent.

reyang

LGTM, I've left some minor suggestions.

Add skeleton metrics exporter to azure

Add skeleton metrics exporter to azure add bracket (census-instrumentation#690) Implement Azure Metrics Exporter (census-instrumentation#693) fix rst doc for Azure exporter bump version

Add skeleton metrics exporter to azure add bracket (census-instrumentation#690) Implement Azure Metrics Exporter (census-instrumentation#693) fix rst doc for Azure exporter bump version fix comment

Add metrics exporter to azure

cb7f2cc

lzchen requested review from a team, c24t, reyang and songy23 as code owners June 6, 2019 21:35

googlebot added the cla: yes label Jun 6, 2019

lzchen added 2 commits June 6, 2019 14:37

include changelog

41e71a9

fix lint

ec6f7ea

reyang reviewed Jun 6, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Outdated Show resolved Hide resolved

reyang reviewed Jun 6, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Outdated Show resolved Hide resolved

reyang reviewed Jun 6, 2019

View reviewed changes

contrib/opencensus-ext-azure/opencensus/ext/azure/metric_exporter/__init__.py Outdated Show resolved Hide resolved

fix docs, add examples

12a0da6

reyang reviewed Jun 6, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Outdated Show resolved Hide resolved

reyang reviewed Jun 6, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Outdated Show resolved Hide resolved

reyang reviewed Jun 6, 2019

View reviewed changes

contrib/opencensus-ext-azure/opencensus/ext/azure/metric_exporter/__init__.py Outdated Show resolved Hide resolved

fix naming

54f8945

reyang reviewed Jun 7, 2019

View reviewed changes

Fix examples

93af575

lzchen added 3 commits June 10, 2019 14:08

copy stats to metrics

7ef2ff1

Merge branch 'master' of https://github.com/census-instrumentation/op…

79a0cf0

…encensus-python into metrics

package name

ab059f9

switch to stats

fb2f4cf

reyang reviewed Jun 12, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Outdated Show resolved Hide resolved

reyang reviewed Jun 12, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Outdated Show resolved Hide resolved

reyang reviewed Jun 12, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Outdated Show resolved Hide resolved

reyang reviewed Jun 12, 2019

View reviewed changes

opencensus/metrics/export/metric_producer.py Show resolved Hide resolved

lzchen added 7 commits June 12, 2019 16:09

reorder README

a1e940f

fix log to logs

d2b6c36

fix lint

fda6204

remove spaces

5c7adae

lessen line length

ca6a0dc

change examples

6e38ff6

line indents

41a807f

reyang reviewed Jun 13, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Show resolved Hide resolved

reyang reviewed Jun 13, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Show resolved Hide resolved

reyang reviewed Jun 13, 2019

View reviewed changes

reyang suggested changes Jun 13, 2019

View reviewed changes

reyang reviewed Jun 13, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Show resolved Hide resolved

reyang reviewed Jun 13, 2019

View reviewed changes

contrib/opencensus-ext-azure/README.rst Show resolved Hide resolved

reyang reviewed Jun 13, 2019

View reviewed changes

reyang approved these changes Jun 13, 2019

View reviewed changes

move validation

89ea447

lzchen force-pushed the metrics branch from 5dc85ba to 89ea447 Compare June 13, 2019 21:35

lzchen added 4 commits June 13, 2019 14:41

fix merge conflicts

c1cbad6

revert logs_exporter

bdd8141

README

2c2d45b

fix merge

a0a98de

reyang merged commit 0ec23a6 into census-instrumentation:master Jun 14, 2019

lzchen added a commit to lzchen/opencensus-python that referenced this pull request Jul 15, 2019

Skeleton for Azure Metrics Exporter (census-instrumentation#678)

f52c1ec

Add skeleton metrics exporter to azure

lzchen added a commit to lzchen/opencensus-python that referenced this pull request Jul 18, 2019

Skeleton for Azure Metrics Exporter (census-instrumentation#678)

1d9d2d5

Add skeleton metrics exporter to azure

lzchen added a commit to lzchen/opencensus-python that referenced this pull request Jul 19, 2019

Skeleton for Azure Metrics Exporter (census-instrumentation#678)

155a9b7

Add skeleton metrics exporter to azure

c24t pushed a commit that referenced this pull request Jul 24, 2019

Azure exporter release (#678) (#723)

a7308a0



		def new_metrics_exporter(**options):
		options = Options(**options)

Conversation

lzchen commented Jun 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

lzchen commented Jun 7, 2019

Uh oh!

lzchen commented Jun 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reyang Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

lzchen Jun 12, 2019 •

edited

Loading

reyang Jun 13, 2019 •

edited

Loading