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

Sampling interval #2

Closed
wants to merge 7 commits into from
Closed

Sampling interval #2

wants to merge 7 commits into from

Conversation

joehoyle
Copy link
Member

Taken from phacility#80, same patch but refreshed. Needs accreditation.

@Slach
Copy link

Slach commented Apr 29, 2018

Failed to build with php7 sampling-interval branch with following errors

/opt/xhprof/extension/xhprof.c:2025:19: error: 'zval' has no member named 'type'
   } else if(values->type == IS_STRING) {
                   ^
Makefile:193: recipe for target 'xhprof.lo' failed
make: *** [xhprof.lo] Error 1

full log here
https://pastebin.com/mheCFree
;(

joehoyle and others added 3 commits April 30, 2018 20:28
This applies the correction that debug_print_backtrace() uses
internally.
@nathanielks
Copy link

I'm curious why the tests were removed?

@joehoyle joehoyle changed the base branch from master to experimental August 22, 2018 18:42
@nathanielks
Copy link

Excuse me, it was just one test file! Nevermind!

@nathanielks nathanielks requested a review from rmccue August 22, 2018 18:42
@joehoyle
Copy link
Member Author

@nathanielks this PR had the wrong base, I've updated that now, but it wasn't intended to be merged anyway.

@nathanielks nathanielks removed the request for review from rmccue August 22, 2018 18:43
@nathanielks
Copy link

I'll leave it be 😄

@rmccue
Copy link
Member

rmccue commented Aug 23, 2018

@joehoyle I feel like we should just merge this and start maintaining it as a full repo, given we're running in production.

@rmccue
Copy link
Member

rmccue commented Aug 23, 2018

And by "merge this" I mean, merge sampling-interval into experimental, and merge experimental into master. I'd say we should probably also rip out the non-extension directories.

I can get in touch with Phacility again if we want?

@joehoyle
Copy link
Member Author

@rmccue yeah I think that's a good idea. No idea what Phacility are doing!

Also, MediaWiki it looks like chose not to go this route as they were seeing large overheads, which I have not been able to replicate (see https://phabricator.wikimedia.org/T176916). They are exploring other ways to instrument function enter / exits. It's worth us continuing to monitor what solution they come up with as we are essentially wanting to do the same thing (low-overhead flame graphs)

@joehoyle joehoyle closed this Nov 24, 2023
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.

4 participants