Skip to content

Span Inferrer: Fix path for API Gateway V1 #262

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 2 commits into from
Feb 23, 2022
Merged

Span Inferrer: Fix path for API Gateway V1 #262

merged 2 commits into from
Feb 23, 2022

Conversation

hpetru
Copy link
Contributor

@hpetru hpetru commented Feb 22, 2022

What does this PR do?

This Pull Request fixes the span inferrer for API Gateway V1 events.

Motivation

Because of this bug, resources are not shown right in DataDog API Gateway service.

Testing Guidelines

I wrote a until test which replicates the bug.

Additional Notes

Looks like this problem happens only for API Gateway V1, but I also added an additional test case for API Gateway V2 just to make sure that my changes are not breaking something V2 related.

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)

@hpetru hpetru requested a review from a team as a code owner February 22, 2022 23:13
@hpetru
Copy link
Contributor Author

hpetru commented Feb 22, 2022

@astuyve Please take a look and let me know if anything else is needed to get this bug fix merged and a new version released.

@astuyve
Copy link
Contributor

astuyve commented Feb 22, 2022

Thanks so much for the PR @hpetru - the code looks good, re-reading my PR I think I just missed this mid-refactor, so I appreciate your contribution!

Please run yarn lint and re-push, I'll re-run the specs and then cut a new release.

@codecov-commenter
Copy link

Codecov Report

Merging #262 (f31d9bf) into main (5bd4206) will increase coverage by 0.49%.
The diff coverage is 100.00%.

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
+ Coverage   82.26%   82.75%   +0.49%     
==========================================
  Files          37       37              
  Lines        1618     1618              
  Branches      349      349              
==========================================
+ Hits         1331     1339       +8     
+ Misses        238      231       -7     
+ Partials       49       48       -1     
Impacted Files Coverage Δ
src/trace/span-inferrer.ts 90.84% <100.00%> (+5.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bd4206...c8c49e6. Read the comment docs.

@astuyve
Copy link
Contributor

astuyve commented Feb 23, 2022

Hi @hpetru - the integration tests might be tricky to update for you as you'll need both a DD API key as well as an available AWS account.

I'll merge your branch into one of my own, update the tests, and merge that PR, which should solve this. Thanks again for your contribution!!

@astuyve astuyve merged commit fbaa5a3 into DataDog:main Feb 23, 2022
@astuyve
Copy link
Contributor

astuyve commented Feb 23, 2022

This has been released to 5.71.0! We don't adhere to semver here as we keep the minor version in sync with the lambda layer version available on AWS. Unfortunately AWS doesn't allow semver for lambda layers. v71 is also published in all regions for all runtimes.

https://github.com/DataDog/datadog-lambda-js/releases/tag/v5.71.0

@@ -55,7 +55,7 @@ export class SpanInferrer {
): SpanWrapper {
const options: SpanOptions = {};
const domain = event.requestContext.domainName;
const path = event.rawPath || event.requestContext.routeKey;
const path = event.rawPath || event.requestContext.path || event.requestContext.routeKey;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@astuyve I'm afraid that the right change here should be:

const path = event.rawPath || event.requestContext.resourcePath || event.requestContext.routeKey;

Copy link
Contributor

@astuyve astuyve Feb 23, 2022

Choose a reason for hiding this comment

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

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