Implement Azure Metrics Exporter by lzchen · Pull Request #693 · census-instrumentation/opencensus-python

lzchen · 2019-06-21T23:21:49Z

This PR implements the Metrics Exporter for Azure Monitor. The underlying logic uses the Stats API.

Uses asynchronous polling of metrics on a different thread periodically to be exported to Azure Monitor.
Recording of measures is done synchronously
Histograms (ValueDistributions) are not supported since to view metrics in Azure Monitor as histograms, you use queries to construct the summary view that you want
Label Key/Value are inserted into Azure Monitor as "properties"

contrib/opencensus-ext-azure/opencensus/ext/azure/common/protocol.py

…into metrics3

contrib/opencensus-ext-azure/opencensus/ext/azure/common/protocol.py

…into metrics3

contrib/opencensus-ext-azure/examples/metrics/sum.py

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

reyang · 2019-06-24T20:27:57Z

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

+            # which contains the aggregated value
+            for point in time_series.points:
+                if point.value is not None:
+                    data_point = DataPoint(ns=metric.descriptor.name,


Would the name allow something like a namespace? E.g. https://github.com/census-instrumentation/opencensus-specs/blob/master/stats/HTTP.md#measures
If yes, we might consider to split the name string to namespace and the actual name?

Interesting solution. Given the max length of name is 1024 characters, it is possible to encode the namespace and actual name inside the "name" field. Having a namespace field is useful as it categorizes what kind of metric its for, and the name serves as the specific type of metric. However there must be some documentation on how to use the encoding and for corner cases as well. (e.g. if we do name=:<actual_name>, what happens when either of them is blank or the ":" is missing?)

I'd say we leave it as it is for now, I'd rather change the DataPoint API than having to encode something in name.

reyang

Looks good in general. Need to have the test cases and pass CI.

reyang · 2019-06-24T20:35:11Z

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

+            if metric.descriptor.type == MetricDescriptorType.CUMULATIVE_DISTRIBUTION:
+                continue
+            envelopes.append(self.metric_to_envelope(metric))
+            self._transmit(envelopes)


What if transmission failed?

How to control the max batch size (number of items) that we export?

Since we do not have a defined retry logic for Metrics yet, the exporter simply ignores the transmissions that have failed and logs an error message with the given status code. I do not want to throw and exception and stop the application if a single transmission fails.

We have not yet defined what "number of items" means in terms of metrics (# of metrics, time series or points?). The injestion service also only accepts a single metric at a time, the list passed in is simply for convention (so match with stats design). I say we can keep it as it is for now (sends all time series passed in) until we can define what this means.

_transmit has dependency on the storage, we need to decouple it (or make storage optional if we don't use it in metrics exporter).

"sending all the time series passed in" won't work since the ingestion would start to reject data if we hit a certain limit.

Yes you are correct. Leaving the logic to run into the exception is not a good practice. I fixed it so that it will only pass to storage if storage is being used (not being used for Metrics exporter). This means that for now, any partial success response we get back from the ingestion service will be discarded.

The ingestion service only accepts a single data point at a time, so there is no need for batching.

https://github.com/microsoft/ApplicationInsights-Home/blob/master/EndpointSpecs/Schemas/Bond/MetricData.bond

The ingestion service only accepts a single data point at a time, so there is no need for batching.

It is hard to believe that the ingestion service would be able to serve real customer in production without batching.

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

reyang · 2019-06-27T05:29:57Z

contrib/opencensus-ext-azure/opencensus/ext/azure/common/storage.py

        return blob.put(data, lease_period=lease_period, silent=silent)
+
+
+class LocalNoopStorage(object):


This is like a mock class for testing, do we want to hack this way?

Refactored to exporter to include _transmit functionality without retry policy.

contrib/opencensus-ext-azure/README.rst

reyang · 2019-06-27T18:44:42Z

contrib/opencensus-ext-azure/examples/metrics/sum.py

+                                              "number of requests",
+                                              "requests")
+NUM_REQUESTS_VIEW = view_module.View("Number of Requests",
+                                     "View for number of requests made",


Remove "View of".

reyang · 2019-06-27T18:45:52Z

contrib/opencensus-ext-azure/examples/metrics/sum.py

+    view_manager.register_view(NUM_REQUESTS_VIEW)
    mmap = stats_recorder.new_measurement_map()
    tmap = tag_map_module.TagMap()
+    tmap.insert("url", "website.com")


Need to be concise here.

reyang · 2019-06-27T18:51:19Z

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

+        # We assume the ordering is already correct
+        for i in range(len(metric_descriptor.label_keys)):
+            if time_series.label_values[i].value is None:
+                value = "None"


Think about how to make it generic.

reyang · 2019-06-27T18:53:00Z

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

+        envelope.data = Data(baseData=data, baseType="MetricData")
+        return envelope
+
+    def _transmit_without_retry(self, envelopes):


Leave a todo comment here.

The TODO should cover:

consolidate with transport logic (instead of duplicating code)

handle failures properly

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

reyang · 2019-06-27T21:47:57Z

contrib/opencensus-ext-azure/README.rst

            mmap.measure_int_put(CARROTS_MEASURE, 1000)
            mmap.record(tmap)
-            time.sleep(10)
+            time.sleep(60)


If the application exits without sleeping, what's the expected result?
What's our recommendation to the users?

It is possible that an application will exit before the exporter thread has a chance to exporter the metrics based on exporter_interval.

It is recommended for the user to keep their applications running, or at least long enough for the exporter to meet the interval.

For applications that terminate before the default interval (15s), it is fine because we only have up to one minute of granularity for the ingestion service (soon to be 30s), so the data seen prior to one minute does not have much value.

It is recommended for the user to keep their applications running, or at least long enough for the exporter to meet the interval.

This is vague, what does "long enough" mean? We probably want to document it.

The minimum requirement, we don't want to tell the user to "pick some lucky number, wait and pray".

Right. Pit of success.

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

reyang · 2019-06-27T21:52:24Z

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

+                            self._transmit_without_retry(envelopes)
+                            del envelopes[:]
+            # If leftover data points in envelopes, send them all
+            if envelopes:


Need to optimize the logic flow here.

The logic here is to send a request as soon as "max_batch_size" amount of metrics have been processed instead of waiting for all of the metrics to be processed.

I think it is minimal optimization, it might be cleaner to just iterate through all the metrics, convert them to envelopes, and then send the envelopes batch by batch. What do you think?

I was thinking about latter, del envelopes[:] and having a separate logic handling the residues outside several nested scope could be hard to maintain.

Up to you. I don't have strong opinion here.

…encensus-python into metrics2

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

lzchen added 3 commits June 20, 2019 17:26

basic implementation

efb0042

fix time

a63925f

include tags, exclude distrubution aggregation

ce8860b

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

googlebot added the cla: yes label Jun 21, 2019

add test

fbb5c44

reyang reviewed Jun 22, 2019

View reviewed changes

contrib/opencensus-ext-azure/opencensus/ext/azure/common/protocol.py Outdated Show resolved Hide resolved

reyang reviewed Jun 22, 2019

View reviewed changes

contrib/opencensus-ext-azure/opencensus/ext/azure/common/protocol.py Outdated Show resolved Hide resolved

reyang reviewed Jun 22, 2019

View reviewed changes

contrib/opencensus-ext-azure/opencensus/ext/azure/common/protocol.py Show resolved Hide resolved

lzchen added 2 commits June 24, 2019 10:06

format protocol, default options

955157b

Merge branch 'metrics2' of https://github.com/lzchen/opencensus-python …

7864d3e

…into metrics3

reyang reviewed Jun 24, 2019

View reviewed changes

contrib/opencensus-ext-azure/opencensus/ext/azure/common/protocol.py Outdated Show resolved Hide resolved

lzchen added 2 commits June 24, 2019 11:51

fix namespace

749e2a6

Merge branch 'metrics2' of https://github.com/lzchen/opencensus-python …

f0c05f5

…into metrics3

reyang reviewed Jun 24, 2019

View reviewed changes

contrib/opencensus-ext-azure/examples/metrics/sum.py Outdated Show resolved Hide resolved

reyang reviewed Jun 24, 2019

View reviewed changes

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

add testS

fe4fec9

reyang reviewed Jun 24, 2019

View reviewed changes

reyang suggested changes Jun 24, 2019

View reviewed changes

reyang reviewed Jun 24, 2019

View reviewed changes

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

lzchen added 6 commits June 24, 2019 14:03

Add more tests

9c59f42

fix spacing

6bec075

fix lint + test case

b190956

fix lint

1837db2

fix lint

7fbb2bb

fix lint

913f2be

fix transport

ebb0985

reyang reviewed Jun 27, 2019

View reviewed changes

lzchen added 3 commits June 27, 2019 11:11

Decouple from TransportMixin

eb49dc7

move to exporter

75f2be7

Fix tests, lint

8546400

reyang reviewed Jun 27, 2019

View reviewed changes

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

reyang reviewed Jun 27, 2019

View reviewed changes

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

reyang reviewed Jun 27, 2019

View reviewed changes

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

lzchen added 2 commits June 27, 2019 12:23

Address comments, fix lint

db8d081

Add tests

e4ec950

reyang reviewed Jun 27, 2019

View reviewed changes

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

reyang reviewed Jun 27, 2019

View reviewed changes

Add tests

dea8eb2

reyang approved these changes Jun 27, 2019

View reviewed changes

lzchen added 5 commits June 27, 2019 16:06

Address comments

469df5d

coverage

06d4aa8

coverage

5c07b10

fix space

d09e69d

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

b42b2cc

…encensus-python into metrics2

reyang merged commit ed28627 into census-instrumentation:master Jun 28, 2019

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

Implement Azure Metrics Exporter (census-instrumentation#693)

700a457

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

Implement Azure Metrics Exporter (census-instrumentation#693)

f017c31

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

Implement Azure Metrics Exporter (census-instrumentation#693)

d94b66a

		return blob.put(data, lease_period=lease_period, silent=silent)


		class LocalNoopStorage(object):

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

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!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

lzchen commented Jun 21, 2019 •

edited

Loading