Update supported versions; checks & tests. #16
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bripkens
suggested changes
Nov 17, 2017
src/tracing/index.js
Outdated
| @@ -39,7 +39,7 @@ function setDefaults() { | |||
|
|
|||
|
|
|||
| function shouldEnableTracing() { | |||
| if (config.tracing && config.tracing.enabled === false) { | |||
| if (config.tracing && config.tracing.enabled === false && exports.supportedVersion(process.versions.node)) { | |||
| logger.info('Not enabling manual tracing as tracing is not enabled via config.'); | |||
There was a problem hiding this comment.
We previously supported manual tracing for all Node.js versions. Why is this now unsupported?
There was a problem hiding this comment.
With the new CLS, we are now limited by the support provided by cls-hooked. This is why I proposed also the engine entry as a warning on installation.
There was a problem hiding this comment.
Additionally, falling back to continuation-local-storage as an alternative for 8.0.0 & 8.1.4 won't work as it's broken on those versions. (same as cls-hooked)
There was a problem hiding this comment.
This restriction shouldn't exist. Manual tracing shouldn't have a dependency on automatic tracing.
There was a problem hiding this comment.
I forgot that this was manual tracing. I'll remove.
src/tracing/index.js
Outdated
| @@ -49,7 +49,7 @@ function shouldEnableTracing() { | |||
|
|
|||
|
|
|||
| function shouldEnableAutomaticTracing() { | |||
| if (config.tracing && config.tracing.enabled === false) { | |||
| if (config.tracing && config.tracing.enabled === false && exports.supportedVersion(process.versions.node)) { | |||
There was a problem hiding this comment.
In this case we should log a different message.
There was a problem hiding this comment.
We can log a message but it would be better if we warned on install via engine: https://docs.npmjs.com/files/package.json#engines
There was a problem hiding this comment.
Nope, we shouldn't warn on install. The sensor even works on Node.js 0.12. IMHO we should consider automatic tracing an optional feature. This is how we treated it before. Also, correctly interpreting the engine field is harder than interpreting a good log message.
There was a problem hiding this comment.
Ok I'll update the log message.
|
👍 |
basti1302
added a commit
that referenced
this pull request
Nov 30, 2020
Here are two coredumps that were were created on CircleCI under Node.js 8: (lldb) bt * thread #1: tid = 0, 0x0000000000915192 node`node::http2::Http2Session::ClearOutgoing(int) + 178, name = 'node', stop reason = signal SIGSEGV * frame #0: 0x0000000000915192 node`node::http2::Http2Session::ClearOutgoing(int) + 178 frame #1: 0x0000000000916047 node`node::http2::Http2Session::OnStreamAfterWriteImpl(node::WriteWrap*, int, void*) + 39 frame #2: 0x0000000000950ee0 node`node::StreamBase::AfterWrite(node::WriteWrap*, int) + 160 frame #3: 0x000000000099f161 node`node::TLSWrap::InvokeQueued(int, char const*) + 193 frame #4: 0x00000000009a01a8 node`node::TLSWrap::EncOut() + 232 frame #5: 0x00000000009a0bfd node`node::TLSWrap::OnAfterWriteImpl(node::WriteWrap*, int, void*) + 125 frame #6: 0x0000000000950ee0 node`node::StreamBase::AfterWrite(node::WriteWrap*, int) + 160 frame #7: 0x000000000095517b node`node::LibuvStreamWrap::AfterWrite(node::WriteWrap*, int) + 27 frame #8: 0x0000000000955dc4 node`node::LibuvStreamWrap::AfterUvWrite(uv_write_s*, int) + 84 frame #9: 0x00000000009c5129 node`uv__write_callbacks(stream=0x00000000041b05b8) + 265 at stream.c:976 frame #10: 0x00000000009c6358 node`uv__stream_destroy(stream=0x00000000041b05b8) + 200 at stream.c:461 frame #11: 0x00000000009bb34a node`uv_run + 8 at core.c:272 frame #12: 0x00000000009bb342 node`uv_run + 277 at core.c:302 frame #13: 0x00000000009bb22d node`uv_run(loop=0x000000000218cda0, mode=UV_RUN_DEFAULT) + 413 at core.c:372 frame #14: 0x00000000008d6b65 node`node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) + 1205 frame #15: 0x00000000008d5da0 node`node::Start(int, char**) + 352 frame #16: 0x00007fd77d7ed2e1 libc.so.6`__libc_start_main + 241 frame #17: 0x000000000089f601 node`_start + 41 (lldb) bt * thread #1: tid = 0, 0x0000000002c198e0, name = 'node', stop reason = signal SIGSEGV * frame #0: 0x0000000002c198e0 frame #1: 0x0000000000916047 node`node::http2::Http2Session::OnStreamAfterWriteImpl(node::WriteWrap*, int, void*) + 39 frame #2: 0x0000000000950ee0 node`node::StreamBase::AfterWrite(node::WriteWrap*, int) + 160 frame #3: 0x000000000099f161 node`node::TLSWrap::InvokeQueued(int, char const*) + 193 frame #4: 0x00000000009a01a8 node`node::TLSWrap::EncOut() + 232 frame #5: 0x00000000009a0bfd node`node::TLSWrap::OnAfterWriteImpl(node::WriteWrap*, int, void*) + 125 frame #6: 0x0000000000950ee0 node`node::StreamBase::AfterWrite(node::WriteWrap*, int) + 160 frame #7: 0x000000000095517b node`node::LibuvStreamWrap::AfterWrite(node::WriteWrap*, int) + 27 frame #8: 0x0000000000955dc4 node`node::LibuvStreamWrap::AfterUvWrite(uv_write_s*, int) + 84 frame #9: 0x00000000009c5129 node`uv__write_callbacks(stream=0x0000000002d70078) + 265 at stream.c:976 frame #10: 0x00000000009c6358 node`uv__stream_destroy(stream=0x0000000002d70078) + 200 at stream.c:461 frame #11: 0x00000000009bb34a node`uv_run + 8 at core.c:272 frame #12: 0x00000000009bb342 node`uv_run + 277 at core.c:302 frame #13: 0x00000000009bb22d node`uv_run(loop=0x000000000218cda0, mode=UV_RUN_DEFAULT) + 413 at core.c:372 frame #14: 0x00000000008d6b65 node`node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) + 1205 frame #15: 0x00000000008d5da0 node`node::Start(int, char**) + 352 frame #16: 0x00007fde4be1a2e1 libc.so.6`__libc_start_main + 241 frame #17: 0x000000000089f601 node`_start + 41
willianpc
added a commit
that referenced
this pull request
Apr 6, 2021
# This is the 1st commit message: Added basic test apps # This is the commit message #2: improved test app a little bit # This is the commit message #3: more refactoring; wip: process, concurrency named process in test app # This is the commit message #4: added consumer as process # This is the commit message #5: wip: in the middle of another refactoring # This is the commit message #6: test app covers all 12 cases # This is the commit message #7: started instrumentation # This is the commit message #8: wip: instrumenting add() function # This is the commit message #9: wip: dealing with repeatable jobs # This is the commit message #10: Using repeat with limit. avoid using cron # This is the commit message #11: added bulk job sending # This is the commit message #12: small refactoring in sender test app # This is the commit message #13: exit span instrumented for single and repeatable jobs # This is the commit message #14: wip: bulk immediate jobs [skip ci] # This is the commit message #15: wip: testing the instrumentation of bulk > send job [skip ci] # This is the commit message #16: complete instrumentation of exit spans # This is the commit message #17: docs(changelog): prepare release 1.117.0 [skip ci] # This is the commit message #18: v1.117.0 # This is the commit message #19: fix: bypass native addon loading in worker threads Co-authored-by: Willian Carvalho <willian.carvalho@instana.com> # This is the commit message #20: docs(changelog): prepare release 1.117.1 [skip ci] # This is the commit message #21: v1.117.1 # This is the commit message #22: test(tracing): make batching test less flaky # This is the commit message #23: chore: add script for splitting CI output according to package [skip ci] # This is the commit message #24: fix: update to shimmer@1.2.1 to be able to patch async functions Specifically, that version includes this fix: othiym23/shimmer@ec15ba2 # This is the commit message #25: test(tracing): fix path for custom tags # This is the commit message #26: wip: added ENTRY span instrumentation # This is the commit message #27: renamed child process.js; removed extra execTime param from /POST; added client HTTP call # This is the commit message #28: immediate and repeatable jobs instrumented correctly [skip ci] # This is the commit message #29: small refactoring/cleanup; [skip ci] # This is the commit message #30: introduced tests [skip ci] # This is the commit message #31: Included test for Callback x Bulk [skip ci] # This is the commit message #32: wip: attempting to instrument master.js [skip ci] # This is the commit message #33: instrument child process with an extra entry span # This is the commit message #34: experimental: activate tracing immediately, without waiting for connection to agent # This is the commit message #35: send spans directly from Bull child process workers and wait for them to be offloaded # This is the commit message #36: fix linting issues # This is the commit message #37: Updating agent uuid from env var info [skip ci] # This is the commit message #38: fixed PID casting to string [skip ci] # This is the commit message #39: no more IPC, write to a file instead # This is the commit message #40: attach trace context to each individual IPC message instead of env var at process start # This is the commit message #41: avoid duplicated bull entry span # This is the commit message #42: fixed naming convention; replaced fs/promises by fs; guarantee that opts object exists # This is the commit message #43: added disabled trace tests # This is the commit message #44: lift the requirement for applying span.disableAutoEnd/span.end() # This is the commit message #45: Running tests for trace disabled and suppressed # This is the commit message #46: all instana data is removed before customer gets the processed job data # This is the commit message #47: Added test to make sure no Instana data is left in Job data # This is the commit message #48: Added withError cases # This is the commit message #49: Skipping tests for Node version < 10 # This is the commit message #50: fix(opentracing): default to type entry when no parent is referenced # This is the commit message #51: docs(changelog): prepare release 1.117.2 [skip ci] # This is the commit message #52: v1.117.2 # This is the commit message #53: fix(metrics): register gc stats loader listener immediately This fixes an issue where GC metrics would not be available when the dependency gcstats.js was already present in node_modules and the activation of the gc metrics module would happen too late. # This is the commit message #54: docs(changelog): prepare release 1.117.3 [skip ci] # This is the commit message #55: v1.117.3 # This is the commit message #56: docs(contributing): document the need to install PostgreSQL headers (Only required for local development.) Plus: Additional info about rate limited OTP when publishing. [skip ci]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the internal list of supported versions, adds a check before initializing tracing and updates tests to follow these changes.