"no function to unwrap" logged when disabling Express instrumentation #970
Comments
|
@nwalters512, just wondering: Do you actually require disabling the instrumentation? Could you explain the usecase? |
|
Turning an instrumentation off/on at runtime is quite risky. It usually works fine in unittest like setups where a single instrumentation is used but in the wild there are quite a lot modules (not only APM tools) which monkey patch this and that. So unwrap may fail or restore the wrong function if there were some other patch afterwards. |
|
Strictly speaking, we don't require it. The intent was to disable it during tests and in lower environments to avoid unnecessary overhead. This is definitely a premature optimization and not based on any profiling. I just saw that a |
|
Thanks for the explanation! I didn't ask to critique the approach - I agree that if the disable method is there, I, as an user, would expect that to work and be reliable. I tend to agree with what @Flarna said. Even providing it is not a good idea: maintenance overhead, flimsy implementations, broken expectations. I don't think any other SIG supports it either. |
|
Linking a previous discussion/PR here for reference: As far as I know there is no official statement/recommendation that disable/enable shouldn't be used. There are a lot cases where it works fine - in special in unittests where the environment is well known and dependencies are usually limited. But personally I would not use it in production to solve some problem. I would prefer to restart the process and skip this instrumentation from start. |
Ack! Yeah, if the unofficial guidance is "don't disable, it may not work or may break things", I would not expect that to be part of the API.
This would certainly be my preference too, but there's a sort of chicken-and-egg problem here. If you store your config in something like AWS Secrets Manager or AWS AppConfig (like we do), then in order to know if OTel should be enabled, you need to require the AWS SDK, which in turn requires I guess that's something that I should ask about explicitly: is ensuring that OTel instrumentation is the first thing to load instrumented packages actually required? In other words, would something like this actually work correctly? const dns = require('dns');
const { DnsInstrumentation } = require('@opentelemetry/instrumentation-dns');
(new DnsInstrumentation()).enable();
dns.lookup('example.org', (err, address, family) => {
console.log('address: %j family: IPv%s', address, family);
});
// Would I get a span/trace for the above, even though `dns` was
// required before the instrumentation was enabled?If we can indeed safely load instrumentation a bit longer after process startup, that would remove our need to |
|
For the node.js built in modules like dns the above sequence will most likely work (I haven't tried) because these modules are not cached. But a small change can break this: const { lookup } = require('dns');
const { DnsInstrumentation } = require('@opentelemetry/instrumentation-dns');
(new DnsInstrumentation()).enable();
lookup('example.org', (err, address, family) => {
console.log('address: %j family: IPv%s', address, family);
});==> The As e.g. http module uses dns it's needed to look into node.js source code to find out if the require the one or the other way. In my opinion the only production safe variant to disable an instrumentation is to implement all wrapped functions like this: function wrappedFunc(...args) {
if (instrumentation.disabled) {
return Reflect.apply(originalFunc, args)
}
// create spans,..
}Clearly any started but not yet ended span will not disappear, bound callbacks, bound |
What version of OpenTelemetry are you using?
@opentelemetry/api:1.0.4@opentelemetry/instrumentation-express:0.28.0What version of Node are you using?
What did you do?
Reproduction available at https://github.com/nwalters512/otel-js-express-repro
Steps to reproduce:
yarn installnode index.jsWhat did you expect to see?
I do not expect to see anything logged.
What did you see instead?
I see the following output:
Additional context
There's some weird asymmetry in the instrumentation: the
patchcallback effectively invokesthis._wrap(moduleExports.Router, 'route'), whereas theunpatchcallback invokesthis._unwrap(moduleExports.Router.prototype, 'route'). I would expect to see them either both usemoduleExports.RouterormoduleExports.Router.prototype.I don't know enough about how patching works to tell if that's intentional or not, and I can't see anything useful from
git blame. If changingunpatchto not use the prototype is the right solution, I'd be happy to open a PR!The text was updated successfully, but these errors were encountered: