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

feat: sampling feature for logger #7

Merged
merged 4 commits into from
Feb 20, 2020

Conversation

danilohgds
Copy link
Contributor

Issue #, if available:
This relates to issue Enhancement Suggestion - Log Sampling

Description of changes:
This pull request introduces a new variable/ constructor parameter to enable debug log sampling of a given percentage, similar to what DAZN powertools has. POWERTOOLS_LOGGER_SAMPLE_RATE accepts in 0-1 range, to log in debug mode at the given rate.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

.gitignore Show resolved Hide resolved
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking quite good!! Some minor adjustments and one ask to improve functional testing.

@@ -52,15 +59,32 @@ def logger_setup(service: str = "service_undefined", level: str = "INFO", **kwar
>>>
>>> def handler(event, context):
logger.info("Hello")
:param service:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

62-64 doesn't seem right - Can you remove it?

@@ -26,13 +29,17 @@ def logger_setup(service: str = "service_undefined", level: str = "INFO", **kwar
service name
LOG_LEVEL: str
logging level (e.g. INFO, DEBUG)
POWERTOOLS_LOGGER_SAMPLE_RATE: str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant float in type annotation here - POWERTOOLS_LOGGER_SAMPLE_RATE: float

@@ -26,13 +29,17 @@ def logger_setup(service: str = "service_undefined", level: str = "INFO", **kwar
service name
LOG_LEVEL: str
logging level (e.g. INFO, DEBUG)
POWERTOOLS_LOGGER_SAMPLE_RATE: str
samping rate ranging from 0 to 1, float precision
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop ", float precision" and use "1 being 100% sampling" instead. This along with float type annotation should clear out any ambiguity.

if sampling_rate and random.random() <= float(sampling_rate):
log_level = logging.DEBUG
except ValueError:
logger.debug(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raise the exception instead so they know they need to take corrective actions.

raise ValueError(f"Expected a float value ranging 0 to 1, but received {sampling_rate} instead. Please review POWERTOOLS_LOGGER_SAMPLE_RATE environment variable.")

@@ -110,6 +134,8 @@ def logger_inject_lambda_context(
-------
decorate : Callable
Decorated lambda handler
:param log_event:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop these two - I believe these are coming from your IDE automatically ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, pycharm being cheeky :)

log_level = os.getenv("LOG_LEVEL") or level
logger = logging.getLogger(name=service)

# sampling a small percentage of requests with debug level, using a float value 0.1 = 10%~
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this in the right place? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, good catch.

python/tests/functional/test_logger.py Show resolved Hide resolved
…g LOG_LEVEL.

comments/docstrings from IDE removed.
except ValueError:
raise ValueError(
f"Expected a float value ranging 0 to 1, but received {sampling_rate} instead. Please review "
f"POWERTOOLS_LOGGER_SAMPLE_RATE environment variable."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge this line with the above instead of having two lines ;)

f"Expected a float value ranging 0 to 1, but received {sampling_rate} instead. Please review POWERTOOLS_LOGGER_SAMPLE_RATE environment variable."

python/tests/functional/test_logger.py Show resolved Hide resolved
python/tests/functional/test_logger.py Show resolved Hide resolved
minor reformat removal ( pycharm pep8 complains with 120 characters on line )
@heitorlessa heitorlessa merged commit ccd9f03 into aws-powertools:develop Feb 20, 2020
@heitorlessa heitorlessa added the feature New feature or functionality label Jun 3, 2020
@heitorlessa heitorlessa changed the title Sampling feature feat: sampling feature for logger Jun 3, 2020
heitorlessa referenced this pull request in heitorlessa/aws-lambda-powertools-python Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants