Skip to content

Sampling interval#2

Closed
joehoyle wants to merge 7 commits into
experimentalfrom
sampling-interval
Closed

Sampling interval#2
joehoyle wants to merge 7 commits into
experimentalfrom
sampling-interval

Conversation

@joehoyle

Copy link
Copy Markdown
Member

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

@Slach

Slach commented Apr 29, 2018

Copy link
Copy Markdown

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
;(

Slach added a commit to Slach/xhprof that referenced this pull request Apr 29, 2018
see humanmade#2
and phacility#80

Signed-off-by: Slach <bloodjazman@gmail.com>
joehoyle and others added 3 commits April 30, 2018 20:28
This applies the correction that debug_print_backtrace() uses
internally.
@nathanielks

Copy link
Copy Markdown

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
Copy Markdown

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

@nathanielks nathanielks requested a review from rmccue August 22, 2018 18:42
@joehoyle

Copy link
Copy Markdown
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
Copy Markdown

I'll leave it be 😄

@rmccue

rmccue commented Aug 23, 2018

Copy link
Copy Markdown
Member

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

@rmccue

rmccue commented Aug 23, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
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