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

Feature request: main implementation, randomization, middy middleware, correlation ID's, optional: object merging #139

Closed
saragerion opened this issue Jul 27, 2021 · 7 comments
Assignees
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility

Comments

@saragerion
Copy link
Contributor

saragerion commented Jul 27, 2021

Description of the feature request

Problem statement

There are some areas that should be revisited or implemented.

Summary of the feature

  1. We should update the implementation of randomisation of sampling - currently implemented like this:
  private setLogsSampled(): void {
    const sampleRateValue = this.getSampleRateValue();
    // TODO: revisit Math.random() as it's not a real randomization
    this.logsSampled = sampleRateValue !== undefined && (sampleRateValue === 1 || Math.random() < sampleRateValue);
  }

Not optimal.

  1. Implementation of middy-compatible middleware logic to match the one of the decorator which is already existing.

  2. Correlation ID's propagation - see Feature request: correlation ID's propagation #129

  3. Optional: replace lodash library with in-house custom code

Code examples

Benefits for you and the wider AWS community

Describe alternatives you've considered

Additional context

Related issues, RFCs

#129

@saragerion saragerion added this to the beta-release milestone Jul 27, 2021
@saragerion saragerion changed the title logger: business logic implementation - Randomization, middy middleware, correlation ID's logger: business logic implementation - Randomization, middy middleware, correlation ID's, optional: object merging Jul 27, 2021
@bahrmichael
Copy link
Contributor

Writing down my understanding from our discussion yesterday:

  1. We have tests where Math.random yields a spread (or entropy) that's too large (i.e. 50 to 150). We should find some random that reduces the entropy, preferably to something like 90 to 110. This should not increase the bundle size significantly.
  2. Decorators only work for classes, but many people use functional programming with TypeScript, and as far as I understand we can't decorate functions (which are not part of classes). Therefore we want to investigate if we can add logging/tracing/metrics via middy middleware. Example pseudo code below:
app
    .use(cors())
    .use(tracing()
    .use(metrics({coldStarts: true})
  1. No further details, as there's a full issue: Feature request: correlation ID's propagation #129
  2. Lodash was used because it was easy to get started. It's a big library though, so we'd like to replace it with something simpler and smaller.

@dreamorosi
Copy link
Contributor

Regarding number 4, a quick win until we have our own implementation could be to switch to lodash.merge and lodash.clonedeep instead of importing the full package.

Just for context, these two are the functions that I saw are used in the Logger and that are exported as separate packages.

@saragerion
Copy link
Contributor Author

@bahrmichael your understanding is 100% correct.

@dreamorosi well spotted! I wasn't aware that separated packages formats exist.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2021

⚠️ 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 dreamorosi reopened this Aug 9, 2021
@dreamorosi dreamorosi added the logger This item relates to the Logger Utility label Aug 11, 2021
@dreamorosi
Copy link
Contributor

Middy middleware implemented in #313

@saragerion
Copy link
Contributor Author

2, 4 have been addressed.

1 won't be part of the GA milestone and 3 has its own issue.

@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 dreamorosi added feature-request This item refers to a feature request for an existing or new utility completed This item is complete and has been merged/shipped labels Nov 14, 2022
@dreamorosi dreamorosi changed the title logger: business logic implementation - Randomization, middy middleware, correlation ID's, optional: object merging Feature request: main implementation, randomization, middy middleware, correlation ID's, optional: object merging 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 feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility
Projects
None yet
Development

No branches or pull requests

4 participants