Skip to content

Conversation

@94noni
Copy link
Contributor

@94noni 94noni commented Jul 27, 2023

Pull Request

Related issue

Fixes #268 (target #266 so review only 2nd commit)

What does this PR do?

  • Add some raw root code for meilisearch bundle datacollector logic

Feel free to discuss/challenge the implementation/data/design etc


Symfony v5.4

Capture d’écran 2023-08-03 à 19 25 44 Capture d’écran 2023-08-03 à 19 25 54 Capture d’écran 2023-08-03 à 19 26 03 Capture d’écran 2023-08-03 à 19 26 11 Capture d’écran 2023-08-03 à 19 26 23

Symfony v6.3

Capture d’écran 2023-08-03 à 19 28 43 Capture d’écran 2023-08-03 à 19 28 50

@94noni 94noni force-pushed the datacollector branch 3 times, most recently from 4a5f7ae to c3cedff Compare August 3, 2023 17:30
@94noni 94noni force-pushed the datacollector branch 2 times, most recently from f5b0967 to a128f67 Compare August 3, 2023 17:50
Copy link
Collaborator

@norkunas norkunas left a comment

Choose a reason for hiding this comment

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

Please rebase and how does it look like in performance tab if you call multiple functions that are traced with stopwatch?

Comment on lines +22 to +23
}
public function collect(Request $request, Response $response, \Throwable $exception = null): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
public function collect(Request $request, Response $response, \Throwable $exception = null): void
}
public function collect(Request $request, Response $response, \Throwable $exception = null): void

Comment on lines +70 to +71
->addArgument(new Reference('debug.stopwatch'))
;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
->addArgument(new Reference('debug.stopwatch'))
;
->addArgument(new Reference('debug.stopwatch'));

$container->setDefinition('meilisearch.service', $searchDefinition->setPublic(true));
$container->setAlias('search.service', 'meilisearch.service')->setPublic(true);

if ($container->getParameter('kernel.debug')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd instead introduce debug.xml where you configure these things and import the file if it's debug mode :)

{% endfor %}
</div>
<p class="help">
You can also see the HTTP Client & Performance{% if constant('Symfony\\Component\\HttpKernel\\Kernel::VERSION') >= '6.1.0' %} & Serializer{% endif %} tabs to learn more.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the string version comparing really works? Maybe better to use Kernel::VERSION_ID?

@94noni
Copy link
Contributor Author

94noni commented Aug 9, 2023

@norkunas thx for reviewing!
i will be afk mostly for current month but be sure I will keep track of this PR as much as I can, until finishing it
will ping you anyway

edit: not so much free time available lately so PR stale on my side :s

@94noni 94noni changed the title Add datacollector for debug [BLOCKED] Add datacollector for debug Aug 9, 2023
@tacman
Copy link

tacman commented Jun 27, 2024

this looks great, I played around with it a little bit and can't wait until it's merged. What's the reason it's blocked?

@norkunas
Copy link
Collaborator

@tacman it's outdated, needs to be rebased and finished

@tacman
Copy link

tacman commented Jun 27, 2024

I don't know much about rebasing using the git commands.

But the changes here are mostly new files, plus some small tweaks in registering the services. I can create a new fork from the current main branch(?) with the changes from this PR if that would get it merged.

@norkunas
Copy link
Collaborator

I can create a new fork from the current main branch(?) with the changes from this PR if that would get it merged.

👍 do it, I'll review :)

@94noni
Copy link
Contributor Author

94noni commented Jun 27, 2024

hey guys @tacman @norkunas glad it takes traction as I cant push it forward on my side
would be also available for review if needed

cheers

@tacman your PR title contains a typo, its 94noni not 94nomi ;)

@94noni 94noni changed the title [BLOCKED] Add datacollector for debug [STALE] Add datacollector for debug Jun 27, 2024
@tacman
Copy link

tacman commented Jun 27, 2024

@94noni -- my efforts at giving you the credit you rightly deserve!

In fact, I used your code as an template for an unrelated bundle. It's a pain to debug the debugger, but once it works, the Symfony debug toolbar is an amazing tool.

@94noni
Copy link
Contributor Author

94noni commented Jun 27, 2024

allow me to close here then
please feel free to ping me on the other PR when ready for review :)

@94noni 94noni closed this Jun 27, 2024
@94noni 94noni deleted the datacollector branch June 27, 2024 15:09
@tacman
Copy link

tacman commented Jun 27, 2024

@94noni -- I believe it's ready for review! I've only tested it with php 8 and Symfony 7, though

@norkunas
Copy link
Collaborator

norkunas commented Jul 3, 2024

@94noni @tacman I can complete this PR, but I have one question about this:

A single method from search service, could be called more than once, and each call would override previous call data. Does this make sense?

@94noni
Copy link
Contributor Author

94noni commented Jul 3, 2024

Hello this is indeed relevant i didnt thought about that back then
Perhaps each data[function] should be array as well to buffer each individual call if more than one

@tacman
Copy link

tacman commented Jul 3, 2024

Indeed, I think there are several things to tweak, I ended up not even using the bundle because my entities aren't in doctrine.

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.

[RFC] add data collector for profiler and debug

3 participants