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: upgrade opentelemetry to v0.17 #22570

Closed
wants to merge 10 commits into from

Conversation

weyert
Copy link

@weyert weyert commented Feb 26, 2021

This pull request upgrades opentelemetry to the latest version which should implement the 1.0 Trace Spec that came out on 16 February 2021.

This PR attempts to resolve issue #22574

@weyert weyert changed the title feat: upgrade opentelemetry to v0.17 DRAFT: feat: upgrade opentelemetry to v0.17 Feb 26, 2021
@weyert weyert changed the title DRAFT: feat: upgrade opentelemetry to v0.17 feat: upgrade opentelemetry to v0.17 Feb 26, 2021
tapico-weyert and others added 8 commits February 26, 2021 17:06

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@timneutkens timneutkens mentioned this pull request Mar 3, 2021
2 tasks
@ijjk ijjk added the type: next label Mar 3, 2021
@kodiakhq kodiakhq bot closed this in #22713 Mar 10, 2021
kodiakhq bot pushed a commit that referenced this pull request Mar 10, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
A number of changes here.  I recommend viewing the diff with the <a href="?w=1">whitespace flag enabled</a>.

- OpenTelemetry is replaced with a custom and lightweight tracing solution.
- Three trace targets are currently supported: console, Zipkin, and NextJS.
- Tracing is now governed by environment variables rather than `--require instrument.js`.
  + `TRACE_TARGET`: one of `CONSOLE`, `ZIPKIN`, or `TELEMETRY`; defaults to `TELEMETRY` if unset or invalid.
  + `TRACE_ID`: an 8-byte hex-encoded value used as the Zipkin trace ID; if not provided, this value will be randomly generated and passed down to subprocesses.

Other sundry:

- I'm missing something, probably a setup step, with the Zipkin target.  Traces are captured successfully, but you have to manually enter the Trace ID in order to view the trace - it doesn't show up in queries.
- I'm generally unhappy with [this commit](235cedc).  It is... untidy to provide a telemetry object via `setGlobal`, but I don't have a ready alternative.  Is `distDir` strictly required when creating a new Telemetry object?  I didn't dig too deep here.

As noted, there are a lot of changes, so it'd be great if a reviewer could:

- [ ] pull down the branch and try to break it
- [ ] check the Zipkin traces and identify possible regressions in the functionality

Closes #22570
Fixes #22574
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 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.

None yet

3 participants