-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add local storage and retry logic for Azure Metrics Exporter + flush telemetry on exit #845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -35,49 +35,66 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class MetricsExporter(object): | |||
class MetricsExporter(TransportMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[#852]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more blocking comments from me, thanks!
Similar to how
AzureLogHandler
andAzureExporter
have local storage and retry logic, metrics exporter now implements these as well.Also exports all current metrics on application exit.
Resolves [#786]