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

Favor parameterized routes for resource name. #513

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

purple4reina
Copy link
Contributor

What does this PR do?

Set the resource name to the parameterized route key when using api gateway v2.

Motivation

A metric is created from each span made by the tracers. The metric name comes from the operation name. The resource name gets added as a tag. Therefore unbound values for resource name can cause a large increase in the number of metrics on an account.

Testing Guidelines

To test this, I created two apps and pulled the event payloads for each to use in the unit tests.

functions:
  v1:
    handler: handler.handler
    events:
      - http:
          method: get
          path: /user/{id}
  v2:
    handler: handler.handler
    events:
      - httpApi:
          method: get
          path: /user/{id}

Additional Notes

This feature already worked for api gatewawy v1.

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)

@purple4reina purple4reina requested a review from a team as a code owner March 15, 2024 21:10
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 81.67939% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 81.86%. Comparing base (5d49414) to head (93be85b).
Report is 12 commits behind head on main.

❗ Current head 93be85b differs from pull request most recent head 5c1c24d. Consider uploading reports for the commit 5c1c24d to get more accurate results

Files Patch % Lines
src/index.ts 60.00% 6 Missing ⚠️
src/metrics/extension.ts 70.58% 5 Missing ⚠️
src/trace/context/extractors/sqs.ts 61.53% 3 Missing and 2 partials ⚠️
src/trace/span-inferrer.ts 75.00% 1 Missing and 1 partial ⚠️
src/trace/xray-service.ts 92.85% 2 Missing ⚠️
src/utils/log.ts 60.00% 2 Missing ⚠️
src/metrics/listener.ts 94.73% 0 Missing and 1 partial ⚠️
src/trace/patch-http.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
- Coverage   82.10%   81.86%   -0.25%     
==========================================
  Files          54       55       +1     
  Lines        2191     2255      +64     
  Branches      511      521      +10     
==========================================
+ Hits         1799     1846      +47     
- Misses        329      344      +15     
- Partials       63       65       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -94,7 +94,15 @@ export class SpanInferrer {
const options: SpanOptions = {};
const domain = event.requestContext.domainName || "";
const path = event.rawPath || event.requestContext.path || event.requestContext.routeKey;
const resourcePath = event.rawPath || event.requestContext.resourcePath || event.requestContext.routeKey;
var resourcePath = event.rawPath || event.requestContext.resourcePath || event.requestContext.routeKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest refactoring this line + 98 into a new method with your new conditional as the first path, and use early returns as control flow.

My concern would be that we shadow/overwrite resourcePath conditionally here in 101, and that if we add new logic here then someone needs to modify the logic on 97 they may not realize that their change would be later overwritten if it's a parameterized route.

Is that helpful?

@purple4reina purple4reina requested a review from astuyve March 26, 2024 19:44
src/trace/span-inferrer.ts Outdated Show resolved Hide resolved
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.

One note, everything else looks good

@astuyve astuyve merged commit 798bdbf into main Mar 27, 2024
25 checks passed
@astuyve astuyve deleted the rey.abolofia/resource-route branch March 27, 2024 18:04
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.

3 participants