Eliminate Monkey Patching for Diagnostic Instrumentation

[mike-kaufman]

Forked from issue #95 so we can track seperately:

@watson wrote:

Problem: Can we standardise the way tracing information is sent from an instrumented module (called module-x below) to an APM agent?

Suggestions:

• Maybe we can use the V8 trace events API
  - Pros: No need for module-x to know about the APM agents
  - Cons:
  - Currently no way to bind the trace event to the current context
  - Might be expensive to cross the barrier into C-land
  - Might not be detailed enough
  - Overhead of running in production?
• Alternatively invent a new API in JavaScript land
  - This could either be in Node core or a userland module
  - If it’s a userland module, then module-x needs to detect if it’s present before it can send events to it. > The APM agents need to do the same.
  - What if there’s multiple versions of this module installed?
  - Research if other languages have something similar

To frame the problem a bit more concisely: Current APM vendors have to monkey-patch libraries to produce diagnostic data. E.g., if you want to know specific details of a DB query, one would monkey-patch the DB driver to capture any necessary params and stats.

This is problematic because

• monkey-patching is brittle (it necessarily relies on internal implementation details),
• each APM vendor has to effectively do the same thing.
• multiple APM vendors loaded into the same process can trample on each other.
• ESM modules wil require a custom loader to support monkey-patching.

We would like to get to a solution that has the following characteristics:

• de-facto standard APIs for producing & consuming messages. (i.e., major APM vendors should buy into this)
• ability to "phase-in" use of APIs by monkey-patching at first, and then pushing library maintainers to write to the "event sink" internally, thus eliminating needs for monkey-patching.
• API is back-compat (ideally down to Node v4)
• Ability to turn off event publication and then have "near-zero" overhead of any libraries that are leveraging publication APIs.
• a solution that accounts for asynchronous context. That is, data events necessarily need to be correlated with asynchronous context. That said, we shouldn't conflate the problem of "monkey-patching for data events" (what we're tryign to solve here) with "monkey-patching to track async continuations" (addressed by async-hooks).

[AndreasMadsen]

What are the cons of using async_hooks? I can think of a few but was this not discussed?

[Flarna]

AsyncHooks are fine to correlated initiator and corresponding callbacks for native calls. But I don't think it's really possible to extract the data passed to the functions.

Besides that I think AsyncHooks have some blind spots:

• Native extensions not using the AsyncHooks Embedder API (e.a. plain NAN or V8 APIs). This could be improved by integrating AsyncHooks Embedder API to NAN APIs used in such cases.
• APIs which are based on parsing some data stream where there is no 1:1 mapping of outgoing writes and incoming data - in the end AsyncHooks were never designed for such cases.

[AndreasMadsen]

In AsyncHooks the embedder can pass a custom resource object where they can store any information they like. Such as the data passed to a function.

[watson]

Wouldn't you normally only implement the Embedder API on async boundaries - when queueing a callback, just before calling it, just after calling it etc. In APM we need sometimes hooks into other places of the code. For example instrumenting all middleware functions in Express or templating languages that doesn't do any async calls are both quite common.

[AndreasMadsen]

Wouldn't you normally only implement the Embedder API on async boundaries - when queueing a callback, just before calling it, just after calling it etc. In APM we need sometimes hooks into other places of the code.

You could also use the Embedder API for making the context flow easier to understand. A database request, even if there is no queue, can involve a hundred async operations. Wrapping that with async_hooks seems very reasonable.

For example instrumenting all middleware functions in Express or templating languages that doesn't do any async calls are both quite common.

Indeed, for the purely synchronous cases it doesn't make sense to use async_hooks.

---

I think trace_events is the way to go when async_hooks doesn't make sense (actually async_hooks emits trace_events).

Currently no way to bind the trace event to the current context

This we can fix. The V8 team is already working on it.

Might be expensive to cross the barrier into C-land

I've done a great deal of benchmarking with trace_events versus more naive logging. The price of crossing the JS/C++ boundary is offset by the great performance of trace_events logging versus more native logging.

If someone would like to benchmark this for themselves, I have already implemented a basic version of trace_events on process.binding('trace_events'). See usage example here: https://github.com/nodejs/node/blob/master/lib/internal/trace_events_async_hooks.js

Might not be detailed enough

What details do you need? You can emit arbitrary events in trace_events you can also link things together. Yes, by default it only supports having two extra arguments per event, but these can contain strings so one could just JSON.stringify if there are more. A more involved implementation can also support more than two extra arguments.

Overhead of running in production?

For sure trace_events will have the smallest overhead. It was built to be used in production.

[Flarna]

Maybe I haven't fully understood Async Hooks yet. If my task is to trace e.a. a http.get() call or or just a fs.read() . Which data to I get from AyncHooks here?

In our case we usually monitor high level requests e.a. a DB request, not all the 100 async calls needed to get it done. As a result we would like to extract the high level data not the low level protocol messages.

Sure, the 100 async interactions in between need to be tracked also in a lot cases but just to correlate request and cb, without extracting specific data. Here async hooks look really fine.

[danielkhan]

We started working with the OpenCensus folks to create a generic, vendor neutral tracing API. Maybe this is something we can incorporate into Node.js as well. Did anyone already research using anything like OpenCensus, Dapper, OpenTracing? I'd almost expect that, as Google is driving that.
If not, I will put something together over xmas.

[AndreasMadsen]

Maybe I haven't fully understood Async Hooks yet. If my task is to trace e.a. a http.get() call or or just a fs.read() . Which data to I get from AyncHooks here?

async_hooks will as a minimum trace the "atomic" requests. But you could also trace the non-atomic requests. You will typically end up doing that anyway for queues, which is why I don't see a problem with doing that too for non-queue cases.

[mike-kaufman]

Did anyone already research using anything like OpenCensus, Dapper, OpenTracing? I'd almost expect that, as Google is driving that. If not, I will put something together over xmas.

@danielkhan - I'm not aware of anything. Would be great to see something like this, at a minimum just an inventory of different approaches so we can be somewhat systematic. nodejs/node-eps#48 looks like an effort to explose tracing_events through JS, which also may suffice here.

Also, to be clear, we should scope this issue to addressing the "monkey-patching for gathering metadata about API calls" problem. As discussed above, ppl are monkey-patching for two distinct reasons - tracking async context and capturing details about API calls (e.g., params passed to a DB call). Async hooks is intended for the former. I'm not following suggestions to use Async Hooks APIs for the latter, but perhaps I'm missing something. If this is a valid approach, we should list in above list.

[tedsuo]

Ted from OpenTracing here. Would you consider the OpenTracing C++11 API, or a C API? We're in the process of integrating into a number of technologies, now would be a good time to start working with you!

https://github.com/opentracing/opentracing-cpp
Consider dynamic loading as a way to continue to ship node without needing to recompile in order to include a tracer: opentracing/opentracing-cpp#28

[vdeturckheim]

@mike-kaufman if I understand well, you are suggesting we would want some kind of proper overriding tooling in Node.js?

[mike-kaufman]

@vdeturckheim - not sure what you mean by "proper overriding tooling". What's been expressed here is that monkey patching is invasive & fragile - i.e., it relies on internal details of some library, and when those details change, monkey-patching breaks. Consequently, there's a desire to get away from it.

I think we'd like to find some solution where

1. module authors (e.g., DB client libraries) have a consistent way to publish "data events" about interesting things happening in their library.
2. Interested consumers (e.g., APM vendors) can listen for those events & act on them.

[mhdawson]

@tedsuo last year Node.js integrated a trace engine from google and we have been discussing how to add tracepoints etc. I'm not sure if the use of OpenTracing is relevant to this thread but it might be interesting as a discussion on its own. Ideally it we would be able to plug in different trace engines under the APIs that we expose in C and Javascript in Node.js for tracing. I've not read enough on OpenTracing to see how it would fit into that model. If you have some cycles to thing about it and possibly give the diagnostics team a n overview I'd suggest opening a new issue to start that discussion.

[SergeyKanzhelev]

@mike-kaufman thanks for pointing me to this thread.

@danielkhan we are discussing node.js OpenCensus SDK with Google. I think diagnostics channel and OpenCensus are very aligned. We just need to understand the proper layering and requirements of those.