-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: Initial tracing setup (peer deps + utils) #13899
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
base: main
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
cc0eef5
to
7f08f16
Compare
|
7f08f16
to
1472161
Compare
6ce43ea
to
293392b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. As discussed on Slack, it might be nice if users had the ability to modify the root span (e.g. via a requestHook
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks a lot for adding this!
Just had a minor concern about performance of dynamically importing @opentelemetry/api
multiple times but I think overall, the implementation is great!
import { noop_tracer } from './noop.js'; | ||
import * as load_otel from './load_otel.js'; | ||
|
||
describe('validateHeaders', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If I had to guess, this should be
describe('validateHeaders', () => { | |
describe('get_tracer', () => { |
* @returns {Promise<T>} | ||
*/ | ||
export async function record_span({ name, tracer, attributes, fn }) { | ||
const SpanStatusCode = await load_status_code(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since load_status_code
always dynamically imports @opentelemetry/api
, this might be a performance issue. I'm not 100% sure though because I don't know if dynamic imports are cached. If they are, no problem, if not, you could manually cache the imported module instead instead.
span.recordException({ | ||
name: 'HttpError', | ||
message: error.body.message | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does a HttpError
also have a stack
? If so, would be cool to also record the stack trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, I'm not 100% sure if this applies here but in our SDK, when we capture HttpError
s, we ignore 4xx errors because usually you don't want to record exceptions for bad requests, 404s, etc. Might be worth a thought if they are valuable or too noisy but happy to leave the call up to you!
Adds
@opentelemetry/api@^1.0.0
as an optional peer dependency of SvelteKit, and adds some utility functions we'll use to add first-party tracing support.As of this PR, adding
@opentelemetry/api
to your project will do nothing.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits