-
Notifications
You must be signed in to change notification settings - Fork 231
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
Improve collector view #652
Improve collector view #652
Conversation
87bd733
to
6e0bc70
Compare
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions. |
Is there anything that I can do to make this PR OK ? |
I don't have enough time to review things at the moment, so there isn't much you can do. I'll mark this so the bot doesn't keep reminding you that I don't have time 😞 |
I totally understand. I thought that you were waiting for some changes. Good luck and happy holiday season 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alcaeus This feature would quite useful to many people. I was actually starting working on something similar but then found this PR. Is there any chance you could find a moment to take a look on that again? 🙏🏻
99c7508
to
6874044
Compare
May I ask you some screenshots? I would definitly love to have this improved UI as well, thanks :) |
Of course, I have just updated PR's description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the email change reverted 😊
<td class="nowrap">{{ loop.index }}</td> | ||
<td class="nowrap">{{ '%0.3f'|format(command.durationMicros / 1000) }} ms</td> | ||
<td class="nowrap">{{ command.database }}</td> | ||
<td>{{ command.command }}</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raw JSONs are hard to read so I think we could add a collapsible pretty-print version of the command JSON here just like in ElasticaBundle: https://github.com/FriendsOfSymfony/FOSElasticaBundle/blob/master/src/Resources/views/Collector/elastica.html.twig#L129-L135
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about decoding and encoding current value in template to keep canonical extended JSON format ? Like {{ data|json_decode|json_encode(constant('JSON_PRETTY_PRINT')) }}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. FYI, json_decode
has been done in DataCollector since Twig does not provide a such filter, cf. twigphp/Twig#824.
@@ -68,7 +68,9 @@ | |||
<td class="nowrap">{{ loop.index }}</td> | |||
<td class="nowrap">{{ '%0.3f'|format(command.durationMicros / 1000) }} ms</td> | |||
<td class="nowrap">{{ command.database }}</td> | |||
<td>{{ command.command }}</td> | |||
<td> | |||
<pre>{{ command.command|json_encode(constant('JSON_PRETTY_PRINT')) }}</pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should show the non-pretty-print one first and allow to expand it for the pretty print one. You can use data-toggle-selector
attributes to achieve it. See https://github.com/doctrine/DoctrineBundle/blob/2.4.x/Resources/views/Collector/db.html.twig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done by using existing queries-table
and sql-runnable
CSS classes.
The CI failed :
Is it due to |
I'll have to investigate what's going on here: 1.10.12 does not exist, and the build specifically calls for installing 1.5.0. For some reason, the version matches what we had above for pecl, so I'm inclined to think there's a separate issue going on. I'll take a look. |
bfb536b
to
9bec393
Compare
I've updated the target branch to 4.4.x and rebased, which of course adds psalm errors. @franmomu would you be able to add the array shapes to this so we can wrap it up? Thanks! |
There you go! |
You're a legend, @franmomu! Thanks @Jean-Beru and @IonBazan for getting this one done! 🎉 |
This PR makes the collector show execution time of commands. Style has been taken from DoctrineBundle and could be improved in future PR (eg: a better command display).
We may add a better command display later. Eg:
db.User.find({ _id: 1 });
.