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

Fix cardinality of opentelemetry span names on client-side navigation #64852

Open
wants to merge 16 commits into
base: canary
Choose a base branch
from
Open

Fix cardinality of opentelemetry span names on client-side navigation #64852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
878cf30
Fix cardinality of opentelemetry span names on client-side navigation
PatNeedham Apr 22, 2024
7f5044b
Merge branch 'canary' into fix/telemetry-client-navigation
PatNeedham Apr 29, 2024
c9f8807
Merge branch 'canary' into fix/telemetry-client-navigation
PatNeedham Apr 29, 2024
12e9ed1
Merge branch 'canary' into fix/telemetry-client-navigation
samcx Apr 29, 2024
87c14e4
app router(client-side navigation) otel e2e test
PatNeedham Apr 29, 2024
79b528a
Merge branch 'canary' into fix/telemetry-client-navigation
PatNeedham Apr 29, 2024
6006658
fix lint error
PatNeedham Apr 29, 2024
5c82b10
Merge branch 'canary' into fix/telemetry-client-navigation
PatNeedham May 2, 2024
86334ee
more closely align client-side nav test with neighboring ones
PatNeedham May 2, 2024
500a79e
test trace modifications
PatNeedham May 2, 2024
63bbd54
all trace attributes good except parentId
PatNeedham May 2, 2024
f262d02
omit hard-coded and unnecessary id
PatNeedham May 2, 2024
7f1326e
traceId and parentId modifications
PatNeedham May 2, 2024
0527990
Merge branch 'canary' into fix/telemetry-client-navigation
PatNeedham May 16, 2024
a026264
change to snapshot for test assertion
PatNeedham May 16, 2024
e28b95c
Merge branch 'canary' into fix/telemetry-client-navigation
PatNeedham May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/next/src/server/app-render/app-render.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,8 @@ async function renderToHTMLOrFlightImpl(
res,
}

getTracer().getRootSpanAttributes()?.set('next.route', pagePath)
Copy link
Member

Choose a reason for hiding this comment

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

Why move it up? Are some steps below sensitive to it?

Overall, the pagePath is not a great value here and it takes some time to update it to the right value. IMHO, too late.

Copy link
Author

@PatNeedham PatNeedham May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why move it up? Are some steps below sensitive to it?

Yes - the if (isRSCRequest && !isStaticGeneration) { ... } check is where this function can end early, which affects whether next.route gets set.

But also, the snapshot testing looks problematic, for example this is from the most recent PR build-and-test logs:

image

id, parentId, timestamp, traceId & duration values appear dependent on something that's preventing consistent results. Maybe jest.setSystemTime could address the timestamp part? For the others I'm not sure what might ensure that consistency.

Copy link
Member

Choose a reason for hiding this comment

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

We can definitely just use matchAny for timestamp and duration. And probably for traceId. However, id and parentId are not so simple - they represent the nesting hierarchy which is somewhat important for telemetry. We can matchAny them too, but maybe we can find a way to link them up? E.g. maybe we can drop id/parentId, but instead add a synthetic parentName? That way we can test hierarchy as well.


if (isRSCRequest && !isStaticGeneration) {
return generateFlight(ctx)
}
Expand All @@ -900,8 +902,6 @@ async function renderToHTMLOrFlightImpl(
const { ServerInsertedHTMLProvider, renderServerInsertedHTML } =
createServerInsertedHTML()

getTracer().getRootSpanAttributes()?.set('next.route', pagePath)

const renderToStream = getTracer().wrap(
AppRenderSpan.getBodyResult,
{
Expand Down
Loading
Loading