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

Improve docs #5

Merged
merged 19 commits into from
Nov 10, 2023
Merged

Improve docs #5

merged 19 commits into from
Nov 10, 2023

Conversation

tyler36
Copy link
Collaborator

@tyler36 tyler36 commented Nov 6, 2023

No description provided.

@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 7, 2023

@rfay If you have time, could you give this the once over?

@tyler36 tyler36 marked this pull request as ready for review November 7, 2023 03:11
@rfay
Copy link
Member

rfay commented Nov 7, 2023

Sure, it will probably be a day or two.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I made several suggestions, but overall I didn't see real profiling data when I got it going. I only saw basic stuff, not the breakdowns I would expect showing where memory and CPU were used most, and the individual wall times.

It's worth including an xhgui launcher custom command as well so people don't have to go do ddev describe to find the port. And mention in the README that the port will be displayed in ddev describe.

Here's what I see:
image

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rfay
Copy link
Member

rfay commented Nov 7, 2023

This is the kind of detail I was expecting to see (what ddev xhprof provides by default)

image

@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 8, 2023

This is the kind of detail I was expecting to see (what ddev xhprof provides by default)

The kind of detail is available but you need to click through on a specific request.
image

image

@tyler36 tyler36 requested a review from rfay November 8, 2023 06:58
@rfay
Copy link
Member

rfay commented Nov 8, 2023

Thanks for showing me where to click, I clicked on the run, but not on the method. It's probably a good thing to add to the README? Or perhaps that's just the job of the upstream docs.

@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 9, 2023

Thanks for showing me where to click

The first time I got data into xhgui, I too was "underwhelmed"; it was pretty but where was all the good data?

@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 9, 2023

  • added link to an online demonstration (9fd5551)
  • added description with image for "GET" 390da3f

@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 9, 2023

Snuck in the helper command, ddev xhgui to launch (f321f43)

@rfay
Copy link
Member

rfay commented Nov 10, 2023

Test with ddev get https://github.com/tyler36/ddev-xhgui/tarball/20231106_drupal_settings

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks! Now that I know where to click.

  • I note that the volume for the mongo stuff (ddev-d10_xhgui-mongo) doesn't get deleted when the add-on is removed (but it would on ddev delete I think). That might be a thing to do in a follow-up.
  • I am not sure why this uses its own mongo instead of depending on ddev-mongo, but ok...
  • For the xhprof_prepend.php, we just have to get our heads together and think about how to handle it.

Awesome work, huge step forward.

@tyler36
Copy link
Collaborator Author

tyler36 commented Nov 10, 2023

I think there are enough good changes here to merge.
This will make it much easier for people to start using this addon.

All of the documentation feedback was address, and there are issues opened for other implementaion points ( #11, #13 , #14 ).

Thank you for your help and review.

@tyler36 tyler36 merged commit 46d9893 into main Nov 10, 2023
2 checks passed
@tyler36 tyler36 deleted the 20231106_drupal_settings branch November 10, 2023 05:51
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.

2 participants