Skip to content

Lazy load metrics listener #320

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

Merged
merged 8 commits into from
Dec 13, 2022
Merged

Lazy load metrics listener #320

merged 8 commits into from
Dec 13, 2022

Conversation

amymin00
Copy link
Contributor

@amymin00 amymin00 commented Nov 9, 2022

What does this PR do?

Remove loading of metrics listener whenever using the extension. Instead, lazy import metrics listener only when user intends to send a distribution metric. Decreases loading of the extension by 57ms.

Motivation

Reducing layer sizes during serverless week

Testing Guidelines

Ran existing unit tests and integration tests. Ensured metrics are still sent in snapshots when sendDistributionMetric is called.

Uploaded layer version 150 with lazy loading changes and linked two serverless apps each with the layer. One app used the extension to transfer custom metrics, the second used a forwarder. Ran functions from each app several times and checked that metrics popped up appropriately in Datadog.

Custom metrics received through the extension:
Screen Shot 2022-12-13 at 2 12 10 PM

Custom metrics received through a forwarder:
Screen Shot 2022-12-13 at 2 16 50 PM

Additional Notes

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog
  • This PR passes the integration tests (ask a Datadog member to run the tests)

@amymin00 amymin00 requested a review from a team as a code owner November 9, 2022 20:59
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2022

Codecov Report

Merging #320 (e4d7bd2) into main (35295b3) will increase coverage by 0.19%.
The diff coverage is 96.00%.

❗ Current head e4d7bd2 differs from pull request most recent head 7c26b9a. Consider uploading reports for the commit 7c26b9a to get more accurate results

@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
+ Coverage   80.35%   80.55%   +0.19%     
==========================================
  Files          36       37       +1     
  Lines        1761     1779      +18     
  Branches      410      411       +1     
==========================================
+ Hits         1415     1433      +18     
+ Misses        294      293       -1     
- Partials       52       53       +1     
Impacted Files Coverage Δ
src/index.ts 87.96% <94.73%> (+1.41%) ⬆️
src/metrics/extension.ts 100.00% <100.00%> (ø)
src/utils/extension-path.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@astuyve astuyve left a comment

Choose a reason for hiding this comment

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

LGTM! We can release once we test against our self monitoring apps and confirm the behavior, but the integration tests passing here are already a strong indicator. Nice work!

@amymin00 amymin00 merged commit 47c5508 into main Dec 13, 2022
@amymin00 amymin00 deleted the amy/lazy-load-metrics-listener branch December 13, 2022 19:35
joeyzhao2018 added a commit that referenced this pull request Dec 20, 2022
astuyve pushed a commit that referenced this pull request Dec 20, 2022
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.

4 participants