feat(node): Add shouldCreateSpanForRequest option
#6055
Conversation
| const shouldCreateSpan = (url: string): boolean => { | ||
| if (options?.shouldCreateSpanForRequest === undefined) { | ||
| return true; | ||
| } | ||
|
|
||
| return options.shouldCreateSpanForRequest(url); | ||
| }; |
There was a problem hiding this comment.
I think we should cache the results of shouldCreateSpanForRequest. On the browser side, I can't decide if it's worth it, but in node where bundle size isn't a concern but being able to most efficiently handle lots of incoming requests quickly is, I think we definitely should (the same way we're doing with tracePropagationTargets just below).
There was a problem hiding this comment.
Since lru_map is already a dependency for the ContextLines integration, should I use that rather than an alternative that could potentially grow indefinitely?
If we cache the result of shouldCreateSpanForRequest we should probably mention this in the jsdocs/docs since it's not obvious and users may think they can use shouldCreateSpanForRequest to dynamically stop span generation.
There was a problem hiding this comment.
I just used a map for now!
There was a problem hiding this comment.
Sorry, missed your question. You raise two good points. I think for now a map is simple and hasn't bitten us on the header front, so is unlikely to bite us here, either. If it does, I'm sure someone will let us know! And good point about the docs.
…uldCreateSpanForRequest
Co-authored-by: Katie Byers <lobsterkatie@gmail.com>
…rRequest' into feat/node-add-shouldCreateSpanForRequest
lobsterkatie
changed the title
shouldCreateSpanForRequest option and don't add spans when it returns falseshouldCreateSpanForRequest option
As per the summary here, this PR adds support for an optional
shouldCreateSpanForRequestfunction in the options. When it's defined and returns false, spans will not be attached.