Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Hooks to enable observability and debugging use cases. #3632

Open
owais opened this issue Jan 21, 2020 · 3 comments
Open

Feature request: Hooks to enable observability and debugging use cases. #3632

owais opened this issue Jan 21, 2020 · 3 comments

Comments

@owais
Copy link

owais commented Jan 21, 2020

Feature discussion / request

1. Explain what is your use case

To enable enhanced observability like detailed contextual logging and tracing. Projects like OpenTelemetry, Jaeger, Prometheus etc and paving the way for next gen observability for both cloud-native and traditional setups. One of the biggest hurdles in extracting maximum possible benefit from such systems is that the applications need to be instrumented. It gets even harder when it comes to adding observability to third party libraries such as knex. It would be great if knex could add first class support for open-standards observability systems or at least add facilities to enable others to do so.

This will not only help existing users of Knex understand how their application interacts with the databases but also make Knex a great choice for teams trying to pick an ORM for their new projects.

2. Explain what kind of feature would support this

Knex could have first class support for obervability by directly support open-standards like OpenTelemetry (previous OpenCensus and OpenTracing) but I think it might be better for Knex to just add enough facilities so systems like OpenTelemetry can easily auto-instrument the library at run time. This would remove any dependencies on the OpenTelemetry project and allow people to use alternate open or proprietary observability systems. This can immensely help in detecting erroneous usage and performance bottlenecks.

We'll need to ship two features to support such use cases:

a. Hooks

Add hooks at interesting places so application code can easily collect data. For example, we could have a preConnection, postConnection, preQueryCompile, postQueryCompile, preExecute, postExecute. Application code can then supply callbacks for these hooks and generate spans or logs to record all operations.

b. Context propagation

In order to link parent operations to it's children, Knex will need to pass down some contextual information all downstream operations. This can be done by changing the internal Knex APIs to explicitly pass a context object (much like Go's context package or Django's request object) or achieved by using async hooks to automatically make the context available to all operations. I personally would prefer the less magical and more explicit context object passing but it's not a huge deal.

Given these two features are implemented, we can then have a separate knex-opentelemetry (replace with your preferred observability solution) package that automatically instruments Knex at run time. Community can also come up with integrations for different observability or debugging systems.

3. Why not completely dynamic instrumentation?

100% dynamic run time instrumentation works but is often very fragile. It can easily break between even minor versions if upstream dependency change in incompatible ways. It's very hard and cumbersome for teams to keep their observability code up to date with latest versions of the libraries they use and as a result people often end up using older versions. With a couple of minor new features, all versions of Knex will always be 100% observability ready at launch. Teams that value observability can rely on Knex to always provide them a better picture of how they use their databases.

4. Give some API proposal, how the feature should work

If the idea makes sense to the maintainers, I'd be more than happy to work on an API proposal and then on the implementation as well.

@elhigu
Copy link
Member

elhigu commented Jan 24, 2020

Many of the hooks you are looking for are already there as knex.on() events and as tarn.js hooks, which can be used to monitor pool functionality very closely.

preConnection/postConnection - tarn has acquire connection hooks with start / success / fail handlers, knex has afterConnect callback and actually there... I'm not sure what you actually mean
with these since, creating new connection is actually pretty rare occasion in knex.

preExecute / postExecute are also handled with start,query, query-success and query-error events.

For compilation there might not be any events, but I don't know either if they would be useful at all. There hasn't been any performance issues in query compiler so far.

If you could specify more about in which stage each event should be triggered and how it would be useful for application, some of these could maybe added.

IIRC Knex events also already have context propagation.

  1. Why not completely dynamic instrumentation?

Also because profiling node applications nowadays is pretty easy.

I agree that current events etc. are quite a mess so I'm not completely against having new well designed ones added for allowing to deprecate older ones. But if there is no use cases which cannot be implemented with current features, I don't know if it is worth of effort to rework those.

@tmc
Copy link

tmc commented Oct 5, 2020

I think if Knex adds support for Async Hooks then this would be a trivial implementation to add -- as of now I think the async context is lost and doing .on("query") for example will not appropriately use async context.

https://nodejs.org/dist/latest/docs/api/async_hooks.html#async_hooks_javascript_embedder_api

@spencerwilson
Copy link

Many of the hooks you are looking for are already there as ... tarn.js hooks

Is there a recommended way to reference the tarn pool? Right now

knex.queryBuilder().client.pool

seems to work, but the Knex docs don't mention being able to call knex.queryBuilder(), or referencing the client or its pool property, so I'm unsure if this is supported usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants