feat(node-core): Add isolateTrace option to node-cron instrumentation#18416
feat(node-core): Add isolateTrace option to node-cron instrumentation#18416Lms24 merged 3 commits intogetsentry:developfrom
isolateTrace option to node-cron instrumentation#18416Conversation
There was a problem hiding this comment.
Hey @sebws thanks for opening this PR and apologies for the late review!
This sounds reasonable to me in general but I would like us to also include an integration test. Primarily because this helps us understand the use case for isolateTrace in our automatic cron instrumentation. Would you mind adding a test? Ideally, one that failed before this change and passes with the change. We already have node-cron integration tests in our node-integration-tests and node-core-integration-tests packages. These should give you a good blueprint.
|
@Lms24 I added a test in |
Lms24
left a comment
There was a problem hiding this comment.
Thanks for adding the test! If you don't mind, yes, please also add it to node-integration-tests.
not sure if it would be preferred to be have this as an option at the individual level, e.g., override type of argArray so a full MonitorConfig could be passed into each .schedule().
I think the current approach is good enough for the time being. We can revisit if we need it in the args directly.
|
these tests fail if isolateTrace is set to false. just duplicated the previous test I had done |
8ef5f09 to
77dc9a3
Compare
|
FYI, I had to rebase your branch because there was a failing e2e test which had nothing to do with your changes. The fix was already merged to |
isolateTrace option to node-cron instrumentation
77dc9a3 to
722cdfa
Compare
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #18416 Co-authored-by: Lms24 <8420481+Lms24@users.noreply.github.com>
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).not sure if it would be preferred to be have this as an option at the individual level, e.g., override type of
argArrayso a fullMonitorConfigcould be passed into each.schedule().however, it's also nice to have a default across all