Skip to content
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

Merged
merged 5 commits into from
Jan 28, 2020

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Jan 21, 2020

Implements a SamplingFilter class for AzureLogHandler.
SamplingFilter extends from the Python Filter 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 the AzureLogHandler by the logging_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)

@@ -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 \
Copy link
Collaborator

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

Copy link
Contributor Author

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):
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

@c24t c24t left a 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!

@@ -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())
Copy link
Member

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.

@lzchen lzchen merged commit 4d1d617 into census-instrumentation:master Jan 28, 2020
@lzchen lzchen deleted the sample branch January 28, 2020 22:16
@lzchen lzchen mentioned this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants