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

Maintenance: move redundant code and functionalities in shared commons package #484

Closed
saragerion opened this issue Jan 18, 2022 · 6 comments
Assignees
Labels
completed This item is complete and has been merged/shipped internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) logger This item relates to the Logger Utility metrics This item relates to the Metrics Utility tracer This item relates to the Tracer Utility

Comments

@saragerion
Copy link
Contributor

saragerion commented Jan 18, 2022

Description of the feature request

The execution of this issue should be divided into 3 steps:

  1. Vet the business logic of Logger, Tracer and Metrics utilities to identify code de-duplication opportunities. Opportunities found so far: cold-start business logic, fetching of environment variables.
  2. Add that logic in commons.
  3. Remove duplicated logic from the single utilities, in favour of the logic from the commons package.

Problem statement

Right now there is a number of functionalities that are repeated throughout the 3 different utilities, all with their own business logic and separated unit tests.
This is not optimal because:

  • The same feature should have the same behaviour across all utilities
  • This unnecessarily increases the cost of maintenance
  • Code duplication inflates the libraries's size unnecessarily, with impact over the Lambda package size and consequently cold-starts

Summary of the feature

See above.

Code examples

Benefits for you and the wider AWS community

Describe alternatives you've considered

Additional context

See here:
https://github.com/awslabs/aws-lambda-powertools-typescript/blob/main/packages/logger/src/config/EnvironmentVariablesService.ts
https://github.com/awslabs/aws-lambda-powertools-typescript/blob/main/packages/logger/src/Logger.ts#L92

Related issues, RFCs

#165

N/A

@saragerion saragerion added the triage This item has not been triaged by a maintainer, please wait label Jan 18, 2022
@saragerion saragerion changed the title All: move duplicated code and functionalities in commons package All: move redundant code and functionalities in shared commons package Jan 18, 2022
@saragerion saragerion added this to the production-ready-release milestone Jan 18, 2022
@alex-m-aws alex-m-aws self-assigned this Jan 21, 2022
@dreamorosi dreamorosi self-assigned this Jan 26, 2022
@dreamorosi
Copy link
Contributor

An internal RFC for centralising the cold start logic in the commons package has been created & reviewed by the team. Will be working on a PR in the coming days.

@dreamorosi
Copy link
Contributor

First batch of changes regarding the cold start heuristic have been implemented in commons and released in v0.6.0, adoption in other utilities will be released with v0.7.0 in the coming days.

@ijemmy
Copy link
Contributor

ijemmy commented Mar 18, 2022

We'll focus on moving the EnvironmentVariable helper class and close this ticket for Production Release.

@saragerion
Copy link
Contributor Author

saragerion commented May 10, 2022

Update about this issue: we addressed successfully the cold start logic separation.
We will think about better separations of concerns and abstractions of env variables when we will address this issue after GA:
#846
If necessary we will open another more scoped issue when we will address this unit of work.
Closing this one.

@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@dreamorosi
Copy link
Contributor

This issue was a dependency of #165.

For the time being I have added #165 in the issue text of #846 as it has a fair bit of context & research on how the environment variables are being used.

@saragerion With that said I would consider deprioritising that issue (currently is labeled as medium) or closing it entirely in favor of #846 similar to what done in this one.

@dreamorosi dreamorosi changed the title All: move redundant code and functionalities in shared commons package Maintenance: move redundant code and functionalities in shared commons package Nov 14, 2022
@dreamorosi dreamorosi self-assigned this Nov 14, 2022
@dreamorosi dreamorosi added logger This item relates to the Logger Utility tracer This item relates to the Tracer Utility metrics This item relates to the Metrics Utility internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) completed This item is complete and has been merged/shipped labels Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) logger This item relates to the Logger Utility metrics This item relates to the Metrics Utility tracer This item relates to the Tracer Utility
Projects
None yet
Development

No branches or pull requests

4 participants