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] Add Laravel event QueryExecuted support (Sentry support) #501

Open
wants to merge 8 commits into
base: 1.7
Choose a base branch
from

Conversation

FandaOG
Copy link

@FandaOG FandaOG commented Sep 20, 2021

Changes proposed in this pull request:

  • Laravel standard event support - QueryExecuted
  • supports Sentry (Sentry bind QueryExecuted event)

@FandaOG FandaOG changed the title [FEATURE] Add Laravel event QueryExecuted support [FEATURE] Add Laravel event QueryExecuted support (Sentry support) Sep 20, 2021
$executionTime = microtime(true) - $this->start;
// TODO get connection name
// this class is Laravel connection wrapper (QueryExecuted needs getName())
$object = new class {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually be a \Illuminate\Database\Connection instance. Though implementing that seems like a pretty huge task.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The laravel-telescope QueryWatcher uses the ->connection->prepareBindings on this event: https://github.com/laravel/telescope/blob/e39423d3ee18898eeb1e0adf2fbe7b74a4cc4996/src/Watchers/QueryWatcher.php

Copy link
Author

@FandaOG FandaOG Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 2 options i guess.

  1. extend my anonymous object (add prepareBindings)
  2. try to implement ConnectionInterface with empty methods. This methots could be extended in future if it will be nessesery.

I will try the 2nd option. Create a wrapper for DoctrineConnection with Laravel ConnectionInterface.

Copy link
Author

@FandaOG FandaOG Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eigan I've maybe found a way. Please check my last commit 0208073

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Could you make some tests for this too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Could you make some tests for this too?

I've check Sentry support and it works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about PHPUnit test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added few tests.

@rosamarsky
Copy link
Contributor

rosamarsky commented Sep 23, 2021

I would do it more cleanly without public properties and with DI for EventDispatcher mocking ability:
https://gist.github.com/rosamarsky/18a6ed0786179c39e8eb3cc6d3ae953a
There is also need to make full filled Illuminate Connection from Dbal Connection before pass it into event.

But this feature is not a Logger. You are dispatching an event, which means that Illuminate Query has been executed and some applications and packages handle this event, and in some cases this can be fatal. So, IMHO, this feature makes sense, but this is not a logger. You can use the Sentry SDK to create a some SentryLogger and send queries through it...

@FandaOG
Copy link
Author

FandaOG commented Sep 24, 2021

@rosamarsky Thank you for your proposal. I used them.

I know it is not a Logger. But I think it fits there. It is optional developers can enable it optionaly. Implement this like a standard event is better because it could support more libraries, solutions... So it solve Sentry SDK support (error tracing and performance monitoring) and it could support Laravel Telescope etc.

{
$this->start = microtime(true);
$this->query = $sql;
$this->params = $params;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using an array as binding (for IN clauses) this will break:

PDO::quote(): Argument #1 ($string) must be of type string, array given

Telescope QueryWatcher tries to quote values that are not int/float:

if ($binding === null) {
  $binding = 'null';
} elseif (! is_int($binding) && ! is_float($binding)) {
  $binding = $this->quoteStringBinding($event, $binding);
}

Using a join() makes it work while keeping the query readable:
image

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

4 participants