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

feat(trace): postprocess trace reporter for datadog #35032

Merged
merged 6 commits into from
Mar 8, 2022

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Mar 3, 2022

This PR is a companion change to e76712f @Brooooooklyn is working on.

Description

PR introduces new script under /scripts, which does post-process trace files generated by performance metrics script. In the script, it parses generated trace output file, then recrated spans with proper chronogical order to send to one of the monitoring system. Currently, it is tied to specific monitoring vendor but constructed spans are mostly opentracing-compatible which can be migrated easily later.

The goals of this script is to observe specific usecase sceanario of next.js (i.e build command) over time in the development branch. It is not intended to each individual manually runs this even though it is techinically possible. Instead, it is expected to setup proper CI actions to trigger.

Given this trace is intended to be used for development branch and not aim to actual end users for the release build, one of the deliberate decisions made in this PR is to do postprocessing generated trace files by reconstructing spans. This allows existing trace implementation under /trace/ as much as non-invasive, also ensure this won't affect any prod users other than development branch. The only changes made to actual next.js codebase is 6b84725 to include proper wall-clock time to reconstruct span. It may possible to revisit some of defined structures of spans to consolidate possibly overlapping properties (i.e now vs. timestamp), but that'll require some more thought around general span improvement.

There aren't automated testing for the changes. You may need to setup proper agent with environments to confirm if script correctly sends traces.

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have helpful link attached, see contributing.md

Documentation / Examples

  • Make sure the linting passes by running yarn lint

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

//
// We have to read through end of the trace -
// There's no gaurantee collected trace have order for its parent span to inner childs, which
// we have to reconstruct all of tree here before send actual traces.
Copy link
Member

Choose a reason for hiding this comment

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

The wording of this sentence is a little confusing. Maybe something like: "Trace events in the input file can appear out of order, so we need to remodel the shape of the span tree before reporting."

padmaia
padmaia previously approved these changes Mar 4, 2022
Copy link
Member

@padmaia padmaia left a comment

Choose a reason for hiding this comment

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

Left a nitpick, but otherwise looks good.

@kwonoj kwonoj force-pushed the remote-trace-adapter branch 2 times, most recently from 5eef10b to 2a7c7a1 Compare March 7, 2022 17:41
@ijjk

This comment has been minimized.

@weyert
Copy link

weyert commented Mar 7, 2022

Is this an internal feature? If not, you could also consider using Opentelemetry which supports multiple exporters including data dog

@kwonoj
Copy link
Contributor Author

kwonoj commented Mar 8, 2022

Is this an internal feature? If not, you could also consider using Opentelemetry which supports multiple exporters including data dog

  1. For now yes, it is internal facing feature
  2. Current PR assumes to reuse existing output without modification as much, reason it does not adapt any other telemetry or standard tracing formats
  3. It may, or may not next.js will introduce different tracing - but I think that's probably out of scope for this PR since it requires public-facing interfaces with discussions.

timneutkens
timneutkens previously approved these changes Mar 8, 2022
Brooooooklyn
Brooooooklyn previously approved these changes Mar 8, 2022
@ijjk
Copy link
Member

ijjk commented Mar 8, 2022

Failing test suites

Commit: 7134f21

yarn testheadless test/integration/image-optimizer/test/old-sharp.test.js

  • with outdated sharp > Server support with next.config.js > should maintain animated png 2
Expand output

● with outdated sharp › Server support with next.config.js › should maintain animated png 2

expect(received).toBe(expected) // Object.is equality

Expected: true
Received: false

  203 |       `inline; filename="animated2.png"`
  204 |     )
> 205 |     expect(isAnimated(await res.buffer())).toBe(true)
      |                                            ^
  206 |   })
  207 |
  208 |   it('should maintain animated webp', async () => {

  at Object.<anonymous> (integration/image-optimizer/test/util.js:205:44)

Read more about building and testing Next.js in contributing.md.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk
Copy link
Member

ijjk commented Mar 8, 2022

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
buildDuration 15s 15.6s ⚠️ +589ms
buildDurationCached 6.1s 6.2s ⚠️ +144ms
nodeModulesSize 372 MB 372 MB ⚠️ +5.11 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
/ failed reqs 0 0
/ total time (seconds) 2.937 3.043 ⚠️ +0.11
/ avg req/sec 851.18 821.67 ⚠️ -29.51
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.303 1.327 ⚠️ +0.02
/error-in-render avg req/sec 1918.47 1884.34 ⚠️ -34.13
Client Bundles (main, webpack)
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
925.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 27.9 kB 27.9 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.5 kB 71.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 192 B 192 B
amp-HASH.js gzip 309 B 309 B
css-HASH.js gzip 327 B 327 B
dynamic-HASH.js gzip 2.57 kB 2.57 kB
head-HASH.js gzip 351 B 351 B
hooks-HASH.js gzip 920 B 920 B
image-HASH.js gzip 5.09 kB 5.09 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.26 kB 2.26 kB
routerDirect..HASH.js gzip 320 B 320 B
script-HASH.js gzip 387 B 387 B
withRouter-HASH.js gzip 319 B 319 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.8 kB 14.8 kB
Client Build Manifests
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
_buildManifest.js gzip 460 B 460 B
Overall change 460 B 460 B
Rendered Page Sizes
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
index.html gzip 531 B 531 B
link.html gzip 545 B 545 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB

Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
buildDuration 17.6s 19s ⚠️ +1.3s
buildDurationCached 5.8s 5.8s ⚠️ +27ms
nodeModulesSize 372 MB 372 MB ⚠️ +5.11 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
/ failed reqs 0 0
/ total time (seconds) 2.913 2.994 ⚠️ +0.08
/ avg req/sec 858.35 834.98 ⚠️ -23.37
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.323 1.326 0
/error-in-render avg req/sec 1889.72 1884.73 ⚠️ -4.99
Client Bundles (main, webpack)
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
925.HASH.js gzip 178 B 178 B
framework-HASH.js gzip 42.3 kB 42.3 kB
main-HASH.js gzip 28.2 kB 28.2 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 72.1 kB 72.1 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 179 B 179 B
amp-HASH.js gzip 313 B 313 B
css-HASH.js gzip 324 B 324 B
dynamic-HASH.js gzip 2.56 kB 2.56 kB
head-HASH.js gzip 351 B 351 B
hooks-HASH.js gzip 921 B 921 B
image-HASH.js gzip 5.23 kB 5.23 kB
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 2.33 kB 2.33 kB
routerDirect..HASH.js gzip 322 B 322 B
script-HASH.js gzip 388 B 388 B
withRouter-HASH.js gzip 317 B 317 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 15 kB 15 kB
Client Build Manifests
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary kwonoj/next.js remote-trace-adapter Change
index.html gzip 530 B 530 B
link.html gzip 544 B 544 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB
Commit: f1949e4

@ijjk ijjk merged commit 8d0561e into vercel:canary Mar 8, 2022
@kwonoj kwonoj deleted the remote-trace-adapter branch March 8, 2022 16:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants