-
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 Fixed-rate sampling logic for Azure Log Exporter #848
Conversation
@@ -116,6 +127,9 @@ class AzureLogHandler(TransportMixin, BaseLogHandler): | |||
def __init__(self, **options): | |||
self.options = Options(**options) | |||
utils.validate_instrumentation_key(self.options.instrumentation_key) | |||
if self.options.logging_sampling_rate < 0 or \ |
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.
nit:You can move this at the beginning of the method to avoid doing anything else if sampling rate is incorrect
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.
Validating the instrumentation key is also a validation and fast fail in itself.
@@ -107,6 +108,16 @@ def stop(self, timeout=None): # pragma: NO COVER | |||
return time.time() - start_time # time taken to stop | |||
|
|||
|
|||
class SamplingFilter(logging.Filter): |
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.
Is there any desire to support adaptive sampling for Application Insights as well? If so, it may be worth it to name this FixedRateSamplingFilter
to be more specific.
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.
Currently in the roadmap there is no plan to add adaptive sampling. If this is ever added, it will be simple to iterate on this since the class is not exposed to customers.
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.
Superficial comments only, LGTM!
contrib/opencensus-ext-azure/opencensus/ext/azure/log_exporter/__init__.py
Outdated
Show resolved
Hide resolved
@@ -207,3 +213,37 @@ def test_log_with_invalid_custom_properties(self, requests_mock): | |||
|
|||
self.assertFalse('not_a_dict' in post_body) | |||
self.assertFalse('key_1' in post_body) | |||
|
|||
@mock.patch('requests.post', return_value=mock.Mock()) |
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.
Surprising to see post
patched here, but the only easy alternative I see is TransportMixin._transmit
, which isn't obviously a better target.
Implements a
SamplingFilter
class forAzureLogHandler
.SamplingFilter
extends from the PythonFilter
logging class. Upon construction of the AzureLogHandler, a filter is added to the handler which filters out log records based off of a random number generated. The fixed rate is passed in to theAzureLogHandler
by thelogging_sampling_rate
parameter which should be in the range [0,1.0]. The default rate if none passed is 1.0.AzureLogHandler(logging_sampling_rate=0.5)