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

Add query-specific uuid to enable finite metrics. #1510

Merged
merged 9 commits into from Jun 17, 2016

Conversation

funnylookinhat
Copy link
Contributor

When trying to profile queries throughout an application, binding to query, query-response, and query-error isn't quite as useful as it could be because there's no way to uniquely identify individual queries that are sent off to the connection.

This adds a (hopefully appropriately) named field of __knexQueryUid to both raw and query-builder queries.

For example:

knex.on('query', function (obj) {
  // Start profiling obj.__knexQueryUid
});
knex.on('query-response', function (res, obj) {
  // Record metrics for obj.__knexQueryUid
});
knex.on('query-error', function (err, obj) {
  // Record metrics for obj.__knexQueryUid
});

This should hopefully resolve this feature request: #1502

I didn't see any tests specific to the events that are emitted when running queries - so I'm not sure if/where I should add tests for this PR.

@wubzz
Copy link
Member

wubzz commented Jun 17, 2016

The tests for the events are awkwardly placed here, in case you wanted to write some.

@funnylookinhat
Copy link
Contributor Author

@wubzz Ah thanks :) I've added what I think should be necessary now.

@funnylookinhat
Copy link
Contributor Author

@wubzz Let me know if you have any feedback. Sorry to spam the PR - but I couldn't get the tests working locally so I just abused Travis. :-P

@wubzz wubzz merged commit acbc8a1 into knex:master Jun 17, 2016
SuperSpyTX added a commit to SuperSpyTX/knex that referenced this pull request Jun 19, 2016
It appears that in Issue knex#1510, unique IDs were added to queries.  The dependency that was used was a development dependency, which isn't installed as a default.  This change makes sure the dependency is installed by default.
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

Successfully merging this pull request may close these issues.

None yet

2 participants