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.

Sign up for GitHub

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

Jump to bottom

fix: Fix latency issue on GPA for no quotes found #375

Merged
merged 6 commits into from
Oct 9, 2024
Merged

fix: Fix latency issue on GPA for no quotes found #375

merged 6 commits into from
Oct 9, 2024

Conversation

zi-yang-uni
Copy link
Collaborator

@zi-yang-uni zi-yang-uni commented Sep 5, 2024 •
edited
Loading

  • Fixes API-486
  • Add handler duration metric
  • Update aws-cdk/aws-redshift-alpha to fix local deploy issues (previous version using nodejs 14 which is deprecated)
  • We are taking a latency hit (~650ms) on logging error for CustomError. If CustomError let's serialize it properly by calling toJSON method. Hopefully this fixes the latency issues we are seeing for NoQuote Error

Field | Value
-- | --
1728437195260
637423166136:/aws/lambda/GoudaParameterizationStack-QuoteE2906A56-yPW9v679o3QJ
2024/10/09/[8]c3216dd7f073472287ade8193f5bb3cc
2024-10-09T01:26:32.697Z	5950ac19-b888-47d8-9808-90ce56eb7aee	INFO	Error logging took 640ms
5950ac19-b888-47d8-9808-90ce56eb7aee
1728437192697

Screenshot 2024-10-08 at 1 55 05 PM

@zi-yang-uni zi-yang-uni force-pushed the ziyiyang/metric-logging branch from 8ea0dd0 to 22b0f71 Compare September 5, 2024 20:19
@zi-yang-uni zi-yang-uni changed the title fix: Move latency metric to api-handler fix: Add handler duration metric to api-handler Sep 5, 2024
@ConjunctiveNormalForm
Copy link
Member

might be a dumb question - how does this differ from the Lambda duration metric that AWS generates automatically for each lambda?
Screenshot 2024-09-05 at 5 06 37 PM

@zi-yang-uni zi-yang-uni force-pushed the ziyiyang/metric-logging branch from 0494e45 to 050bac4 Compare September 5, 2024 21:12
@zi-yang-uni
Copy link
Collaborator Author

@ConjunctiveNormalForm Do we want to cut the latency by the different handlers? I think the lambda duration is total time since start of the lambda execution, while I think we should still be interested in the latency taken by the actual handlers.

dannythedawger
dannythedawger requested changes Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@dannythedawger dannythedawger left a comment

Choose a reason for hiding this comment

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

looks good! Comment for awaiting, but also lets post a SS of this working from your local? These dimensions get nasty so we should make sure it works before we merged!

lib/handlers/base/api-handler.ts Show resolved Hide resolved
@zi-yang-uni zi-yang-uni force-pushed the ziyiyang/metric-logging branch from 050bac4 to 3956a29 Compare October 9, 2024 16:15
@socket-security Socket Security
Copy link

socket-security bot commented Oct 9, 2024 •
edited
Loading

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@aws-cdk/[email protected] environment 0 587 kB aws-cdk-team

🚮 Removed packages: npm/@aws-cdk/[email protected]

View full report↗︎

@zi-yang-uni zi-yang-uni changed the title fix: Add handler duration metric to api-handler fix: Fix latency issue on GPA for no quotes found Oct 9, 2024
@linear Linear
Copy link

linear bot commented Oct 9, 2024

API-486 GPA Latency investigation

ConjunctiveNormalForm
ConjunctiveNormalForm approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@ConjunctiveNormalForm ConjunctiveNormalForm left a comment

Choose a reason for hiding this comment

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

huge!

codyborn
codyborn approved these changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@codyborn codyborn left a comment

Choose a reason for hiding this comment

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

🤞

dannythedawger
dannythedawger reviewed Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@dannythedawger dannythedawger left a comment

Choose a reason for hiding this comment

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

looks good, one comment

cdk.context.json Outdated Show resolved Hide resolved
dannythedawger
dannythedawger reviewed Oct 9, 2024
View reviewed changes
cdk.context.json Outdated Show resolved Hide resolved
dannythedawger
dannythedawger approved these changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@dannythedawger dannythedawger left a comment

Choose a reason for hiding this comment

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

lgtm

@zi-yang-uni zi-yang-uni merged commit 78041dc into main Oct 9, 2024
6 checks passed
@zi-yang-uni zi-yang-uni deleted the ziyiyang/metric-logging branch October 9, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgkol cgkol cgkol approved these changes

@dannythedawger dannythedawger dannythedawger approved these changes

@ConjunctiveNormalForm ConjunctiveNormalForm ConjunctiveNormalForm approved these changes

@codyborn codyborn codyborn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zi-yang-uni @ConjunctiveNormalForm @codyborn @dannythedawger @cgkol