Add local storage and retry logic for Azure Metrics Exporter + flush telemetry on exit#845
Conversation
c24t
left a comment
There was a problem hiding this comment.
Change look good except for a stray print. The class structure including BaseExporter here looks like it could use some attention.
contrib/opencensus-ext-azure/opencensus/ext/azure/metrics_exporter/__init__.py
Outdated
Show resolved
Hide resolved
contrib/opencensus-ext-azure/opencensus/ext/azure/metrics_exporter/__init__.py
Outdated
Show resolved
Hide resolved
contrib/opencensus-ext-azure/opencensus/ext/azure/metrics_exporter/__init__.py
Outdated
Show resolved
Hide resolved
contrib/opencensus-ext-azure/opencensus/ext/azure/metrics_exporter/__init__.py
Outdated
Show resolved
Hide resolved
contrib/opencensus-ext-azure/tests/test_azure_metrics_exporter.py
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| class MetricsExporter(object): | ||
| class MetricsExporter(TransportMixin): |
There was a problem hiding this comment.
MetricsExporter isn't a BaseExporter because we're using the export thread from get_exporter_thread instead of BaseExporter's worker?
It looks like there's a fair amount of duplicated code here, and that BaseExporter should be refactored or made specific to tracing.
There was a problem hiding this comment.
Yes it seems like MetricsExporter and AzureLogHandler have very similar behaviour in terms of the exporter thread. I would like to eventually have all of them be a BaseExporter. Was there any discussions with you and Reiley why this shouldn't be the case (why it was like this in the first place)? In any case, I'd like to keep those refactoring changes separate from a feature PR like this one, but I will cut a ticket open for it.
There was a problem hiding this comment.
Was there any discussions with you and Reiley why this shouldn't be the case (why it was like this in the first place)?
It looks like BaseExporter was factored out in #642, but that PR didn't touch the StackdriverStatsExporter, which influenced the design of the other stats exporters. In any case this can be a problem for another PR.
c24t
left a comment
There was a problem hiding this comment.
No more blocking comments from me, thanks!
Similar to how
AzureLogHandlerandAzureExporterhave local storage and retry logic, metrics exporter now implements these as well.Also exports all current metrics on application exit.
Resolves [#786]