Azure log exporter by reyang · Pull Request #668 · census-instrumentation/opencensus-python

reyang · 2019-05-30T15:36:59Z

Implemented the actual log conversion and exporting logic.
Moved the network/storage transmission logic from trace exporter to a shared mixin, applied to both traces and logs.
Avoid common names (Object and prototype), use BaseObject and _default instead. (Addressing the comments left in Initial version of Azure extension #613 (comment)).

lzchen · 2019-05-30T20:59:03Z

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



-class AzureLogHandler(BaseLogHandler):
+class AzureLogHandler(TransportMixin, BaseLogHandler):


There's some duplicated behaviour in AzureLogHandler and the BaseExporter logic. Ideally we should have the logging behaviour (emit) inherited from logging.Handler, queue/Worker and export inherited from BaseExporter. Why are we not re-using the exporter logic if we are making the log "exporter"?

You're right, that's our goal. For more info please take a look at #642 #657.

Today the trace exporter has legacy issue that needs to be refactored (e.g. trace exporter has export method which sends data to the queue, and emit which actually export the data. what we want is to only have export method, move queue and worker to the core opencensus library).

In this logging effort, we're trying to have a cleaner design, and later we'll refactor traces to follow this design.

The fact that the mixin relies on the exporter's options and storage makes this kind of brittle. I think this is fine for now (and as you mentioned we need to refactor the exporters), but we should revisit this before moving it out of the package.

lzchen · 2019-05-30T21:05:27Z

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



-class AzureLogHandler(BaseLogHandler):
+class AzureLogHandler(TransportMixin, BaseLogHandler):


Should the exporters really be a child class of the Transporter? The two don't really have the "is-a" relationship used for inheritance. Maybe we should have transport be used as a service or utility instead?

Here I took the mixin approach, so the log handler owns all the resources, transport mixin just provides the helper methods on top of these resources.

reyang · 2019-05-31T17:13:10Z

@c24t @songy23 need your help to review, thanks!

songy23

First pass looks good, though I'm not too familiar with Azure data models.

songy23 · 2019-05-31T19:18:07Z

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

+            except Exception:
+                pass
+        if response.status_code == 200:
+            logger.info('Transmission succeeded: %s.', text)


Nit: I feel there's a bit too much logs in this class. Maybe consider reducing logs or using traces with a low sampling rate.

This is at info level, which is intended for debugging only and off by default.
The default log level is WARNING.

songy23 · 2019-05-31T19:19:58Z

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

+        This function should never throw exception.
+        """
+        # TODO: prevent requests being tracked
+        blacklist_hostnames = execution_context.get_opencensus_attr(


Not sure if it's a good idea to put this in the trace execution_context? /cc @c24t WDYT?

Long term goal is to avoid exporters to access execution_context directly, for more info, refer to #573.
In this PR, the code was extracted from the existing trace exporter.

We started storing the blacklist in the context in #289, and probably should have caught it then. I think it's a problem for another PR.

I'll try to address it in a separate PR, there are similar scenarios which we need a consistent solution:

How to prevent traces generated by trace exporters (e.g. during sending data via HTTP) being sent to trace exporter and cause infinite loop.

How to prevent logs/exceptions generated by log exporters/handlers being emitted to the same log handlers and cause infinite loop.

My current thinking is to have the entire worker thread marked as "do not track", so all the spans/logs will have a special tag, and based on this tag we can filter these data out before sending them to exporters.

c24t

Only minor comments, but let's review again before moving these into core.

contrib/opencensus-ext-azure/CHANGELOG.md

c24t · 2019-05-31T20:37:08Z

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

+        This function should never throw exception.
+        """
+        # TODO: prevent requests being tracked
+        blacklist_hostnames = execution_context.get_opencensus_attr(


We started storing the blacklist in the context in #289, and probably should have caught it then. I think it's a problem for another PR.

c24t · 2019-05-31T21:26:23Z

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

+            if self._default == self:
                break
-            self = self.prototype
+            self = self._default


I don't understand what's happening here.

This is trying to mimic the JavaScript prototype behavior, traveling through the prototype chain and get all the key-value pairs.
Do you think assigning to self is evil and we should have another variable?

Maybe not evil, but certainly confusing.

Got it, will give a better name in the next PR.

c24t · 2019-05-31T21:56:41Z

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



-class AzureLogHandler(BaseLogHandler):
+class AzureLogHandler(TransportMixin, BaseLogHandler):


The fact that the mixin relies on the exporter's options and storage makes this kind of brittle. I think this is fine for now (and as you mentioned we need to refactor the exporters), but we should revisit this before moving it out of the package.

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

c24t · 2019-05-31T21:58:32Z

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

+        )
+        execution_context.set_opencensus_attr(
+            'blacklist_hostnames',
+            ['dc.services.visualstudio.com'],


May as well give this the constant treatment too.

Will leave it for now. This will go away when we don't have exporters explicitly maintain the blacklist.

contrib/opencensus-ext-azure/tests/test_azure_log_exporter.py

reyang added 7 commits May 29, 2019 11:43

refactor - extract azure transmission logic to a mixin

7a198c4

rename Object->BaseObject, prototype->_default

cbe5dc2

skeleton of LogRecord->Envelope conversion

c417e39

initial azure monitor log exporter impl

fab18f5

initial support for exception

c28b6b7

support callstack

c3442ac

rename example file

1c1e16a

reyang requested review from a team, c24t and songy23 as code owners May 30, 2019 15:37

googlebot added the cla: yes label May 30, 2019

reyang added 2 commits May 30, 2019 08:52

fix lint

8fe57c3

fix coverage

a6340ef

lzchen reviewed May 30, 2019

View reviewed changes

reyang added 2 commits May 30, 2019 23:01

add coverage test

6fe8bde

skip coverage for race condition

7ad0976

update changelog

c0da6d5

songy23 reviewed May 31, 2019

View reviewed changes

c24t approved these changes May 31, 2019

View reviewed changes

address review comments

c9391b4

reyang merged commit 6901d09 into master May 31, 2019

reyang deleted the azure branch May 31, 2019 23:58



		class AzureLogHandler(BaseLogHandler):
		class AzureLogHandler(TransportMixin, BaseLogHandler):

Conversation

reyang commented May 30, 2019

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 May 30, 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!

reyang commented May 31, 2019

Uh oh!

songy23 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!

c24t left a comment

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!

Choose a reason for hiding this comment

Uh oh!

reyang May 31, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

reyang May 30, 2019 •

edited

Loading

reyang May 31, 2019 •

edited

Loading