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

Papercuts we're currently prioritizing #857

Closed
Tracked by #1009
whardier opened this issue Nov 30, 2021 · 17 comments
Closed
Tracked by #1009

Papercuts we're currently prioritizing #857

whardier opened this issue Nov 30, 2021 · 17 comments
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@whardier
Copy link
Contributor

Need to add proper ContextManager typing via typing.ContrxtManager in several places.

Currently overriding single_metric (-> ContextManager[SingleMetric]) and noticed several other areas where this needs done as well.

Also be nice if there was a proper DummySubSegment type returned for tracing provider.in_subsegment to avoid including types from aws modules to cast.

Expected Behavior

Have rich type annotations along for the ride when using context managers.

Current Behavior

Need to self annotate.

Possible Solution

Add in proper type annotation to some or all of the context manager decorated functions.

@whardier whardier added bug Something isn't working triage Pending triage from maintainers labels Nov 30, 2021
@heitorlessa
Copy link
Contributor

Thanks a lot for raising this @whardier ! I was about to create a list of papercuts we'd like to address, these being two of them.

The SubSegment typing is the most elaborate one, I need one full day to create and refactor a few things, including having an easy way to unit test your application with a Fake Tracer Provider.

@heitorlessa
Copy link
Contributor

When you have a moment, could you help us by enumerating some of them like the SingleMetric ctx manager?

I'm gonna rename this to top papercuts so we can tackle them accordingly as many as possible

image

@heitorlessa heitorlessa changed the title Add ContextManager typing Maintenance: address top papercuts Nov 30, 2021
@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Nov 30, 2021
@heitorlessa
Copy link
Contributor

heitorlessa commented Nov 30, 2021

List of papercuts we're currently prioritizing based on their impact

High impact

Medium impact

Low impact

  • Tracer: Typing SubSegment and Context Manager
  • Tracer: Expose a context manager to trace a block of code instead of suggesting an escape hatch

Maintenance impact

Non-customer facing but that needs to be addressed at some point

  • Batch: Improve maintainability of batch processing functional tests (it should be easier to create tests)
  • Idempotency: Consider refactoring test setup, potentially DynamoDB Local for functional (not integration)

Completed

@heitorlessa heitorlessa added area/event_handlers documentation Improvements or additions to documentation labels Nov 30, 2021
@elliottyates
Copy link

A thought: it took me a few moments and tries to get unit testing working for some features (e.g. #706). One suggestion might be a dedicated area of the docs for unit testing, instead of hunting down the solutions for each feature.

@michaelbrewer
Copy link
Contributor

michaelbrewer commented Dec 1, 2021

@michaelbrewer
Copy link
Contributor

michaelbrewer commented Dec 1, 2021

Docs PR:

@whardier
Copy link
Contributor Author

whardier commented Dec 2, 2021

@michaelbrewer
Copy link
Contributor

yet another one that @heitorlessa found:

@heitorlessa
Copy link
Contributor

Need to create an issue for it

Another one: Idempotency doesn't support serialization of data classes - both input (idempotency key) and return... it could be easily solved by inspecting if it's a dataclass, then using asdict(). asdict() is infamous for having a performance hit on deeply nested dataclasses, so we'll need to mention in the docs.

@heitorlessa
Copy link
Contributor

heitorlessa commented Dec 17, 2021

  • Internal papercut: Improve maintainability of batch processing functional tests (it should be easier to create tests)
  • Internal papercut: Improve maintainability of Idempotency functional tests

@michaelbrewer
Copy link
Contributor

We could already fixes, but definitely fits into this theme:

@michaelbrewer
Copy link
Contributor

michaelbrewer commented Dec 18, 2021

Another one: Idempotency doesn't support serialization of data classes - both input (idempotency key) and return... it could be easily solved by inspecting if it's a dataclass, then using asdict(). asdict() is infamous for having a performance hit on deeply nested dataclasses, so we'll need to mention in the docs.

@heitorlessa see

@michaelbrewer
Copy link
Contributor

michaelbrewer commented Dec 22, 2021

@michaelbrewer
Copy link
Contributor

michaelbrewer commented Dec 23, 2021

Sorry @heitorlessa a papercut fix caused another paper cut :P

@michaelbrewer
Copy link
Contributor

michaelbrewer commented Jan 4, 2022

Some typos and code clean up for data classes:

@aws-powertools aws-powertools locked and limited conversation to collaborators Jan 7, 2022
@heitorlessa
Copy link
Contributor

I'm locking this issue and will split them up into individual issues. We've accidentally got sidetracked in not tackling the actual top papercuts and started addressing new ones.

@heitorlessa heitorlessa pinned this issue Feb 7, 2022
@heitorlessa heitorlessa changed the title Maintenance: address top papercuts Papercuts we're currently prioritizing Feb 7, 2022
@heitorlessa
Copy link
Contributor

Resolving in favour of #1009 so we can keep it clean

@heitorlessa heitorlessa unpinned this issue Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants