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

Backend decoupling #244

Merged
merged 1 commit into from
May 2, 2019
Merged

Backend decoupling #244

merged 1 commit into from
May 2, 2019

Conversation

1ma
Copy link
Contributor

@1ma 1ma commented Jul 8, 2018

Backend Decoupling

PR implementing #234 (Work in Progress)

xhgui-collector change: perftools/xhgui-collector#11

Summary of changes

Xhgui_Profiles was renamed to Xhgui_Searcher_Mongo and an interface (Xhgui_Searcher_Interface) was extracted from it. Then I refactored the bits of the codebase that depended on Xhgui_Profiles to depend on Xhgui_Searcher_Interface instead, and started working on the new Xhgui_Searcher_Pdo. Finally, I tweaked the config and the DI container so that the user can choose which implementation to use when an HTTP request hits the app.

Usage

In config/config.default.php there is a new pdo key:

    'pdo' => array(
        'dsn' => null,
        'user' => null,
        'pass' => null,
        'table' => 'xhgui_profiles'
    ),

The first three fields map to the arguments of the PDO constructor, and the fourth is the table where the profiles are stored. When the existing save.handler key is now set to pdo these values are used. So if you wanted to use an SQLite database, you'd copy the config to config/config.php and put, for instance:

    'save.handler' => 'pdo',

    'pdo' => array(
        'dsn' => 'sqlite:/some/absolute/path/project_db.sqlite3',
        'user' => null,
        'pass' => null,
        'table' => 'xhgui_profiles'
    ),

Keep in mind that you need xhgui-collector to create these profiles, but the same new config applies to it.

Pending work

Methods from Xhgui_Searcher_Pdo:

  • latest()
  • query($conditions, $limit, $fields)
  • get($id)
  • getForUrl($url, $options, $conditions)
  • getAvgsForUrl($url, $search)
  • getAll($options) - /!\ partial, ignores $options atm
  • insert($profile)
  • delete($id)
  • truncate()

Other

  • Refine schema in xhgui-collector (no indexes atm)
  • Test with other databases: all SQL code (schema and queries) would ideally run on SQLite, MySQL and PostgreSQL. This would spare us to create an Xhgui_Seacher_Interface implementation for each platform.

src/Xhgui/Controller.php Show resolved Hide resolved
src/Xhgui/Controller/Run.php Outdated Show resolved Hide resolved
src/Xhgui/Searcher/Interface.php Show resolved Hide resolved
src/Xhgui/Searcher/Pdo.php Show resolved Hide resolved
tests/bootstrap.php Outdated Show resolved Hide resolved
@1ma
Copy link
Contributor Author

1ma commented Jul 8, 2018

No idea why Travis broke, looks like it might not be autoloading any class (I can run the tests successfully).

src/Xhgui/Searcher/Mongo.php Outdated Show resolved Hide resolved
src/Xhgui/Searcher/Pdo.php Outdated Show resolved Hide resolved
src/Xhgui/Searcher/Pdo.php Show resolved Hide resolved
tests/bootstrap.php Outdated Show resolved Hide resolved
src/Xhgui/ServiceContainer.php Outdated Show resolved Hide resolved
src/Xhgui/ServiceContainer.php Outdated Show resolved Hide resolved
@iamkiros
Copy link

Any news, guys?

@GrzegorzDrozd
Copy link

I did not want to mess up this branch code so I started my own attempt to separate UI from storage. I have a working branch with mongo, pdo and file operations (but only storage type at a time). I just need to find some time to finish that up and push.

@1ma
Copy link
Contributor Author

1ma commented Apr 23, 2019

@GrzegorzDrozd The xhgui-collector part is ready. Do you want to finish the job here? (I would close this PR)

@GrzegorzDrozd
Copy link

No. I will create new PR.

@glensc
Copy link
Contributor

glensc commented Apr 24, 2019

collector part is released now: https://github.com/perftools/xhgui-collector/releases/tag/1.7.0

you can omit 7435c57 commit

@glensc
Copy link
Contributor

glensc commented Apr 24, 2019

I don't think 08cbb4e commit belongs to this PR. submit as separate PR if you intend to publish that change.

@1ma
Copy link
Contributor Author

1ma commented Apr 25, 2019

No problem, that's just for me. I used to have my own, uncommited docker-compose.yml file in my local copy of the project. I'll revert that commit once the branch is ready.

@1ma
Copy link
Contributor Author

1ma commented May 1, 2019

This branch is at a stage where the PDO integration is useful (or at least useful enough for my use case), which is:

  • printing the main page with the summary
  • printing the /run/view?id=xxxx page with correct data
  • printing the callgraph

The integration is still rough and incomplete though, but I am stretched thin. What I'd suggest is merging the branch and tagging a new release but saying that this feature is still experimental, then I'd break down the remaining work in small tasks so it can be done incrementally (by me as time allows or by whoever wants to do it).

If anyone else wants to test the PDO integration themselves to assess the current status:

  1. Make sure PHP has the pdo-sqlite extension.
  2. Require perftools/xhgui-collector:^1.7 in any other project of your choice and prepend the header.php file as usual.
  3. In the collector's config.php, set 'save.handler' to 'pdo' and 'pdo'['dsn'] to 'sqlite:/some/local/path/xhgui.sqlite3'.
  4. Make a few test runs, check that xhgui.sqlite3 is created wherever you wanted.
  5. Clone my fork of xhgui, checkout the backend-decoupling branch and set up the webapp as you like (docker, local nginx/Apache whatever). Set up the webapp's config.php the same way as the collector (i.e. pointing to the xhgui.sqlite3 file).
  6. Launch the browser and navigate to the app, you should see the traces and be able to see their details.

@glensc
Copy link
Contributor

glensc commented May 1, 2019

regarding merge technical aspect:

  • the current commits (as of fb39099) is rather dirty

so therefore:

  • do you intend to rebase to fixup/squash some commits?
  • or you prefer to merge the whole branch as single commit (git merge --squash)?

could do the rebase/squash/fixup myself too, but the PR creator has to allow maintainer pushes. not sure how to check is that on or off right now.

@1ma
Copy link
Contributor Author

1ma commented May 1, 2019

I see the "Allow edits from maintainers" checkbox tickled, you mean that?

@glensc
Copy link
Contributor

glensc commented May 2, 2019

yes, that checkbox. but you missed the original question :)

@1ma
Copy link
Contributor Author

1ma commented May 2, 2019

There you go.

@glensc glensc changed the title WIP: Backend decoupling Backend decoupling May 2, 2019
@glensc glensc merged commit f9cc197 into perftools:master May 2, 2019
@1ma 1ma deleted the backend-decoupling branch May 2, 2019 21:56
@glensc
Copy link
Contributor

glensc commented May 2, 2019

merged. thanks! I might even test the result soonish.

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.

6 participants