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

improv adopt logging best practices #23

Merged
merged 23 commits into from
Apr 24, 2020
Merged

improv adopt logging best practices #23

merged 23 commits into from
Apr 24, 2020

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Apr 21, 2020

Issue #, if available: #16 #21

BREAKING CHANGE

logger_setup and logger_inject_lambda_context had to be deprecated in favour of Log class - The user experience remains the same so it's easier swap. If you try to use any of these functions it'll raise an DeprecationWarning exception.

While we're on Beta we strive to make backwards compatible changes, however this wasn't the case - The former implementation impact all loggers (root) and can't be easily updated to allow both decorators to work cleanly, and without side effects.

UX

UX stays the same except we now use a class Logger instead of functions.

from aws_lambda_powertools.logging import Logger

logger = Logger(service="payment", level="INFO") 
# logger = Logger() # implicit config via env vars 

@logger.inject_lambda_context
def handler(event, context)
  logger.info("Collecting payment")
  logger.info({ 
    "operation": "collect_payment",
    "charge_id": event['charge_id'] 
  })

NEW - Appending new keys to structured log

...
@logger.inject_lambda_context
def handler(event, context):
    if "payment_id" in event:
        logger.structure_logs(append=True, payment_id=event["payment_id"])
    logger.info("Collecting payment")

Description of changes:

  • Use a Null Handler to cut noise in customers' logging
  • Create top-level logger aws-lambda-powertools
  • Refactor Lambda Logging module to prevent root logging changes
  • Prevent unnecessary exceptions from Lambda Logging module (e.g. KeyError)
  • Create Logger class and reuse same UX as before
  • Raise deprecated exception for standalone logger_setup, and logger_inject_lambda_context
  • Update docstring
  • Allow appending log keys to structured log
  • Update docs to reflect the change
  • Document package logger and how to enable debug
  • Update tests
  • Talk about deprecation and root logger

Optionally

  • [x] Create a POWERTOOLS_LOG_LEVEL environment variable
  • [x] Set package log level if POWERTOOLS_LOG_LEVEL is set
  • [ ] Set boto logs via env var
    • Best handled by customer's code
  • [ ] Accept an existing logger in logger setup
    • KISS
  • Complete coverage

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

@heitorlessa
Copy link
Contributor Author

@marcioemiranda - Let me know if I missed anything else as I'm working on logger this week to fix logging level along with other suboptimal practices

@heitorlessa heitorlessa changed the title [WIP] Adopt logging best practices Adopt logging best practices Apr 23, 2020
@heitorlessa
Copy link
Contributor Author

@jfuss When you can, may I have your review? This adopts the logging best practices as we discussed

python/aws_lambda_powertools/logging/logger.py Outdated Show resolved Hide resolved
python/aws_lambda_powertools/logging/logger.py Outdated Show resolved Hide resolved
python/aws_lambda_powertools/logging/logger.py Outdated Show resolved Hide resolved
python/aws_lambda_powertools/logging/logger.py Outdated Show resolved Hide resolved
append : bool, optional
[description], by default False
"""
self.handler.setFormatter(JsonFormatter(**self._default_log_keys, **kwargs))
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 a way for a customer to provider their own JsonFormatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - We limit to our own to prevent additional complexity. What we allow is the ability to add additional keys to their structured logger either at logger creation or after.

logger = Logger(request_id="request id!", another="value")
logger = Logger()
logger.structure_logs(request_id="request id", another="value")

Would be happy to hear otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do additional keys have to do with how the Json data is formatted? I am thinking of the case where Company X has a different way they want the data to be formatted, which doesn't have anything to do with what the data actually is in the Json. Or maybe they don't want Json at all and prefer XML (for whatever reason). I don't think this blocks anything here but something to maybe think about in the future.

python/aws_lambda_powertools/metrics/metric.py Outdated Show resolved Hide resolved
python/aws_lambda_powertools/metrics/metrics.py Outdated Show resolved Hide resolved
python/aws_lambda_powertools/middleware_factory/factory.py Outdated Show resolved Hide resolved
@@ -113,6 +114,8 @@ tracer = Tracer(auto_patch=False) # new instance using existing configuration wi

### Logging

> **NOTE** `logger_setup` and `logger_inject_lambda_context` are deprecated and will be completely removed once it's GA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't they kind of already removed since those methods just raise an exception?

@heitorlessa heitorlessa merged commit a99e647 into develop Apr 24, 2020
@heitorlessa heitorlessa deleted the improv/logging branch April 24, 2020 16:34
heitorlessa added a commit that referenced this pull request May 16, 2020
* develop: (21 commits)
  bugfix: #32 Runtime Error for nested sync fns
  chore: renamed history to changelog dependabot
  bugfix: resolves #31 aiohttp lazy import
  chore: grammar issues
  improv: add project tenets
  Improv tracer - async support, patch, test coverage and X-Ray escape hatch (#29)
  Bugfix: "per second" metric units (#27)
  fix: #24 correct example test and docs
  chore: bump example to use 0.8.0 features
  Adopt logging best practices (#23)
  Decorator factory Feat: Create your own middleware (#17)
  chore: clean up CI workflows
  fix: CI attempt 4
  fix: CI attempt 3
  fix: CI attempt 3
  fix: CI attempt 2
  feat: add docs to CI
  chore: fix github badge typo
  chore: pypi monthly download badge
  fix: add missing single_metric example; test var name
  ...
@heitorlessa heitorlessa changed the title Adopt logging best practices improv adopt logging best practices Jun 3, 2020
heitorlessa referenced this pull request in heitorlessa/aws-lambda-powertools-python Jun 17, 2020
* fix: set NullHandler for package logger

* improv: remove root logger logic, lib

* fix: update exception type

* improv: propagate log level if set, null otherwise

* fix: explicit wins over env var

* chore: fix test naming

* fix: exception logging

* improv: shorten log location

* feat: add Logger class wrapper

* improv: add deprecation warning

* BREAKING CHANGE: logger_setup, inject_lambda_ctx

* improv: update tests

* improv: cover duplicated keys edge case

* improv: cover debug package logging

* improv: more coverage, linting

* improv: complete coverage, fix dead code

* docs: update readme to reflect changes

* fix: address jacob's code review feedback

* chore: linting

* fix: metric spillover at 100, not 101

* fix: trace auto-disable, doc edge cases

* chore: linting

* chore: 0.8.0 version bump

Co-authored-by: heitorlessa <lessa@amazon.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants