-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Export TracesSamplerSamplingContext
type
#17523
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
Conversation
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.
Hey @dolbin-prime thanks for opening this PR! Before we think about merging it, can you show me what use case you can't do with the currently exported types and functions?
Would some TS type extraction logic like this work for you?
type TracesSamplerOption = Exclude<Parameters<typeof Sentry.init>[0], undefined>['tracesSampler']
type TracesSamplerSamplingContext = Parameters<Exclude<TracesSamplerOption, undefined>>[0]
function myCustomSampler(ctx: TracesSamplerSamplingContext): number {
return ctx.inheritOrSampleWith(0.3);
}
^ this is obviously not easy to come up with so we can likely just directly export the type. It's public API anyway. Just wanted to check that this is what we're going for.
Also, is it enough for you if this type is only exported from @sentry/core
?
Heads-up: I assigned myself to the PR because I'm reviewing it. Also changed the scope of the PR to feat
since it adds a public export to a package.
TracesSamplerSamplingContext
of types-hoistTracesSamplerSamplingContext
type
Hi @Lms24, thanks for the review! Sorry for the long comment. Any feedback would be greatly appreciated.
Ok, my team is maintaining a monorepo, and there are two kinds of workspace: import { initializeSentry } from '@common/sentry';
import type { TracesSamplerSamplingContext } from '@sentry/core';
function myTraceSampler(ctx: TracesSamplerSamplingContext): number | boolean {
const { name, attributes, inheritOrSampleWith } = ctx;
// app-specific logic
return inheritOrSampleWith(0.1);
}
initializeSentry({
// ...
traceSampler: myTraceSampler,
// ...
});
createRoot(document.getElementById("root")!).render(
<StrictMode>
<App />
</StrictMode>
);
Yeah it works, thank you for your suggestion, but I'm a little concerned about the hard-coded string or magic numbers. I know the name or the number of parameters are unlikey to change, but maybe I'm probably being overly cautious. 😄
I needed the type for my use case and discovered it's declared in |
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.
Thanks for explaining your use case!
Yeah it works, thank you for your suggestion, but I'm a little concerned about the hard-coded string or magic numbers. I know the name or the number of parameters are unlikey to change, but maybe I'm probably being overly cautious
So, the type extraction is public API (just like TracesSamplerSamplingContext
would be) so they're guaranteed to be stable within this major. However, the type extraction indeed goes through a few other properties so it's a little bit more likely to break in a future major version.
That being said, since TracesSamplerSamplingContext
was implicitly already public API, I think it's fine to make it public explicitly now. For now though, let's just export it from @sentry/core
since we rarely export types from core in higher level packages.
I'll merge this PR in a bit. Thanks for contributing!
Thank you for reviewing and merging my PR @Lms24 ! ❤️ |
In #15277,
TracesSamplerSamplingContext
is added to and exported from types-hoist, but is not being exported from index.ts.I'm following the instruction, and I want to inject
(samplingContext: TracesSamplerSamplingContext) => number | boolean
to a function that is wrappingSentry.init
, but I can't becauseTracesSamplerSamplingContext
is not recognized :(