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

Set ProfilingId from Profiler? #39

Closed
glensc opened this issue Sep 4, 2020 · 9 comments
Closed

Set ProfilingId from Profiler? #39

glensc opened this issue Sep 4, 2020 · 9 comments

Comments

@glensc
Copy link
Contributor

glensc commented Sep 4, 2020

In mongo saver there's getLastProfilingId method:

that has a side-effect, that if this saver is called in the application side, the id would be re-used for multiple profilings.

I don't know if that's deliberate or not, but using other savers this functionality is not present.

Question:

  • should the id be generated on the profiler side or xhgui side?

Having it profiler side could overwrite (accidentally or deliberately) existing profiling. I don't think saving the same id twice will merge the results?

Having it set at the profiler side, allows the profiler to control the id value.

and if reading the importer section:

it says importing the same profile twice creates duplicate profiling, so the duplicate id sent from profiler is not overwriting other profilers? is there any point then having value re-used? set from profiler?

cc @perftools/maintainers

@glensc
Copy link
Contributor Author

glensc commented Sep 14, 2020

Considering this issue:

I think there should be no way for the profiler (saver) to specify _id, i.e every submit should be accepted as new profiling submit.

@glensc
Copy link
Contributor Author

glensc commented Sep 24, 2020

Unit tests in xhgui assume the _id can be set by fixture, and therefore the tests fail because PDO driver doesn't allow it:

1) XHGui\Test\Controller\RunTest::testCallgraph
Exception: No profile data found.

@glensc
Copy link
Contributor Author

glensc commented Sep 24, 2020

Allow id-override in pdo saver:

similarly in mongo saver:

but the override is there just to allow testing, at least for pdo.

@markstory
Copy link
Member

It would be simpler if the clients didn't generate ids. That would allow the server and each storage implementation to generate the appropriate ID as required. It also prevents duplicate IDs from being created.

@glensc
Copy link
Contributor Author

glensc commented Oct 8, 2020

thanks, @markstory I've come to the same conclusion, just haven't got around to implementing it so.

@glensc
Copy link
Contributor Author

glensc commented Oct 8, 2020

also, the import method should return the id in the response:

@glensc
Copy link
Contributor Author

glensc commented Oct 12, 2020

ignoring Id from saver if one is provided:

@glensc
Copy link
Contributor Author

glensc commented Oct 22, 2020

Providing id from profiler is no longer supported with latest xhgui (it's ignored).

@glensc
Copy link
Contributor Author

glensc commented Jan 6, 2021

The getLastProfilingId() was added in:

it was a mistake and is removed by now.

a better approach, if need to identify requests and profilers, is to use UNIQUE_ID in env:

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

No branches or pull requests

2 participants