Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Add Fixed-rate sampling logic for Azure Log Exporter#848

Merged
lzchen merged 5 commits intocensus-instrumentation:masterfrom
lzchen:sample
Jan 28, 2020
Merged

Add Fixed-rate sampling logic for Azure Log Exporter#848
lzchen merged 5 commits intocensus-instrumentation:masterfrom
lzchen:sample

Conversation

@lzchen
Copy link
Copy Markdown
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)

@lzchen lzchen requested a review from c24t January 21, 2020 21:10
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
Copy Markdown

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
Copy Markdown
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.

return time.time() - start_time # time taken to stop


class SamplingFilter(logging.Filter):
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

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
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants