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

Fix nextjs resource name #2834

Merged
merged 2 commits into from
Mar 16, 2023
Merged

Fix nextjs resource name #2834

merged 2 commits into from
Mar 16, 2023

Conversation

selimb
Copy link
Contributor

@selimb selimb commented Feb 22, 2023

What does this PR do?

Fixes an issue with the next instrumentation where the resource name for the trace would be wrong when a dynamic route matches a path but isn't actually the one that will handle the page. For instance, given the following directory structure in pages:

api/
├─ hello/
│  ├─ index.js
│  ├─ [name].js
│  ├─ other.js

then a request to /api/other will result in a next.request span with resource.name = GET /api/hello/[name], even though the api/hello/other.js route will be the one to handle the request. Thus, in this case I would expect resource.name to be GET /api/hello/other.

I can create an actual issue for this if you'd prefer.

The bug is shown as a failing test in this PR's first commit (b82e242 - add failing test). Specifically, the test that fails (on all versions) is: for api routes -> should infer the corrrect resource path (/api/hello/other). Note that the new tests in for pages do not fail, but I thought they'd be valuable to have anyway.

The bug is fixed in the next commit (f431c1e - fix test).

Motivation

I wanted accurate resource names in the Trace view on Datadog.

Plugin Checklist

  • Unit tests.

Additional Notes

Heads-up: I believe the next instrumentation will break in v13.1.7 due to Server Router Improvements. Specifically, while handleApiRequest will still exist, this.dynamicRoutes will no longer exist.

However, it looks like handleApiRequest will have the page readily available in its parameters, so it's actually simpler.

When v13.1.7 does get released, I'd be happy to help with updating the instrumentation :). Let me know.

@selimb selimb requested a review from a team as a code owner February 22, 2023 06:26
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #2834 (6639038) into master (a1211b2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2834   +/-   ##
=======================================
  Coverage   89.14%   89.14%           
=======================================
  Files         315      313    -2     
  Lines       11260    11178   -82     
  Branches       33       33           
=======================================
- Hits        10038     9965   -73     
+ Misses       1222     1213    -9     
Impacted Files Coverage Δ
packages/datadog-instrumentations/src/next.js 92.75% <100.00%> (+0.10%) ⬆️

... and 6 files with indirect coverage changes

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

@tlhunter tlhunter force-pushed the nextjs-correct-page branch from f431c1e to 86e57b8 Compare March 1, 2023 17:48
@tlhunter tlhunter force-pushed the nextjs-correct-page branch from 86e57b8 to 6639038 Compare March 1, 2023 18:32
@rochdev rochdev merged commit 6026663 into DataDog:master Mar 16, 2023
@rochdev
Copy link
Member

rochdev commented Mar 16, 2023

Thanks for the PR and sorry for the delay.

When v13.1.7 does get released, I'd be happy to help with updating the instrumentation :).

Do you know if 13.1.7 works properly now that it has been released? Our tests stopped working but that doesn't always mean that the instrumentation did as well, and we didn't really have time to look into it. If it's actually broken please let me know, and also let me know if you want to attempt a fix so that we don't duplicate the work.

@selimb selimb deleted the nextjs-correct-page branch March 17, 2023 11:39
@selimb
Copy link
Contributor Author

selimb commented Mar 17, 2023

@rochdev Thanks for merging!

Do you know if 13.1.7 works properly now that it has been released?

I don't believe 13.1.7 has been released, it looks like it's still in canary. I suspect it's been "promoted" to 13.2.

next 13.2 is not currently supported by dd-trace, so the spans from next don't appear at all. With older versions of dd-trace (e.g. 3.11.0) that didn't have an upper bound on the next version, I end up with GET undefined as the resource name.

And yes, I'll be attempting a fix :)

Might be worth noting that next.js has recently started adding opentelemetry support: vercel/next.js#46198. It's still very early stage though.

@selimb selimb mentioned this pull request Mar 17, 2023
1 task
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
* add failing test

* fix test

---------

Co-authored-by: Selim Belhaouane <selim.belhaouane@garda.com>
@juan-fernandez juan-fernandez mentioned this pull request Mar 28, 2023
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
* add failing test

* fix test

---------

Co-authored-by: Selim Belhaouane <selim.belhaouane@garda.com>
This was referenced Mar 28, 2023
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
* add failing test

* fix test

---------

Co-authored-by: Selim Belhaouane <selim.belhaouane@garda.com>
juan-fernandez pushed a commit that referenced this pull request Mar 28, 2023
* add failing test

* fix test

---------

Co-authored-by: Selim Belhaouane <selim.belhaouane@garda.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants