Fix cardinality of opentelemetry span names on client-side navigation #59282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Leonils
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
feedthejim
approved these changes
Dec 5, 2023
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
1 similar comment
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Failing test suitesCommit: 697f99e
Expand output● app-dir-hmr › filesystem changes › should not break when renaming a folder Read more about building and testing Next.js in contributing.md.
Expand output● opentelemetry › root context › app router › should handle client-side navigation ● opentelemetry › incoming context propagation › app router › should handle client-side navigation Read more about building and testing Next.js in contributing.md. |
|
I can't retrieve the logs of why the pipeline failed. Seems there is a timeout. Do you think it comes from my update ? |
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
Diff detailsDiff for page.jsDiff too large to display Diff for app-page-exp..ntime.dev.jsDiff too large to display Diff for app-page-exp..time.prod.jsDiff too large to display Diff for app-page-tur..time.prod.jsDiff too large to display Diff for app-page-tur..time.prod.jsDiff too large to display Diff for app-page.runtime.dev.jsDiff too large to display Diff for app-page.runtime.prod.jsDiff too large to display |
|
@Leonils are you not able to click on the logs from the CI? let me know if I need to copy them. Seems some of the PPR tests are not passing |
|
On this run I do. I'm investigating. |
|
👋 any update on when this PR will get merged? it's preventing us from adopting OTEL. |
|
4 failing tests / checks are presumably the reason why this can't be merged yet. However when opening those 4, they all display Can anyone with sufficient permissions rerun / re-trigger those tests? I'd like to help out & see what might be causing them to fail |
|
@Leonils I will be closing this |
auto-merge was automatically disabled
April 23, 2024 20:58
Pull request was closed
|
@samcx there was no path to merging this because nobody was able to view how / why the tests were failing - that's why I asked on March 19th if anyone can re-trigger the tests so we can collectively investigate. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
As noticed in issue #54694, span names for dynamic routes are not consistent and sometimes miss the "next.route" attribute, and have a span name that have the parameters instead of the capturing placeholders.
Fixing a bug or a limitation
test/e2e/opentelemetrytest suite)test/e2e/opentelemetryapp used to test telemetry (some pages with link between them where created but not quite correct in the links, and were not handling the defined params)Limits
This PR identify a case where some traces are wrong (see bellow for the detailed identified case). But there might be others that the other of #54694 might think of.
How
Function
renderToHTMLOrFlightImplis the one responsible for rendering of the page. It is also the one that set thenext.routeattribute, the one that has the interpreted route instead of the full url. However, there is a shortcut to exit the function beforenext.routeis set when the conditionisRSCRequest && !isStaticGenerationis met. I think we can setnext.routebefore, so in any case the route has the correct value.Added test
The added test go to page
/app/foo/loading/page1, then click to the<Link/>to/app/[param]/loading/page2. The diff between the expected traces and the ones we get without the fix are:{ ... "name": "render route (app) /app/[param]/loading/page1", ... }, { ... "name": "GET /app/[param]/loading/page1", ... }, { "attributes": { "http.method": "GET", - "http.route": "/app/[param]/loading/page1", "http.status_code": 200, "http.target": "/app/foo/loading/page1?_rsc=4t3jn", - "next.route": "/app/[param]/loading/page1", - "next.span_name": "GET /app/[param]/loading/page1", + "next.span_name": "GET /app/foo/loading/page1?_rsc=4t3jn", "next.span_type": "BaseServer.handleRequest", }, "kind": 1, - "name": "GET /app/[param]/loading/page1", + "name": "GET /app/foo/loading/page1?_rsc=4t3jn", "parentId": undefined, "status": { "code": 0, }, "traceId": "[trace-id]", }, { "attributes": { "http.method": "GET", - "http.route": "/app/[param]/loading/page2", "http.status_code": 200, "http.target": "/app/foo/loading/page2?_rsc=1lrnj", - "next.route": "/app/[param]/loading/page2", - "next.span_name": "GET /app/[param]/loading/page2", + "next.span_name": "GET /app/foo/loading/page2?_rsc=1lrnj", "next.span_type": "BaseServer.handleRequest", }, "kind": 1, - "name": "GET /app/[param]/loading/page2", + "name": "GET /app/foo/loading/page2?_rsc=1lrnj", "parentId": undefined, "status": { "code": 0, }, "traceId": "[trace-id]", }, { "attributes": { "http.method": "GET", - "http.route": "/app/[param]/loading/page2", "http.status_code": 200, "http.target": "/app/foo/loading/page2?_rsc=1yei0", - "next.route": "/app/[param]/loading/page2", - "next.span_name": "GET /app/[param]/loading/page2", + "next.span_name": "GET /app/foo/loading/page2?_rsc=1yei0", "next.span_type": "BaseServer.handleRequest", }, "kind": 1, - "name": "GET /app/[param]/loading/page2", + "name": "GET /app/foo/loading/page2?_rsc=1yei0", "parentId": undefined, "status": { "code": 0, }, "traceId": "[trace-id]", }, { ... "name": "generateMetadata /app/[param]/layout", ... }, { ... "name": "generateMetadata /app/[param]/layout", ... },We see the first load of page1 that has the correct name, but the next render, fetched from client do not.