Install profiling dependencies conditionally #252
Conversation
| @@ -44,6 +44,16 @@ | |||
| $wgShowExceptionDetails = true; | |||
| } | |||
|
|
|||
| // profiling | |||
| // requires docker image to be built with dependencies | |||
| if( isset ( $_SERVER['HTTP_X_WBSTACK_MW_PROFILING'] ) ) { | |||
There was a problem hiding this comment.
This should maybe, probably be hidden behind something more.
There was a problem hiding this comment.
I guess we don't want users of the public service to be able to enable this with a header (even though the actual profiling code won't be baked into their docker image). We could add another environment variable and a check here.
Alternatively we could put this in a separate file and just copy it in for certain builds of the image
There was a problem hiding this comment.
Alternatively we could put this in a separate file and just copy it in for certain builds of the image
+1 sounds like a good approach to me!
There was a problem hiding this comment.
I'm not sure how to conditionally copy this new localsettings file into the container without having to shuffle a bunch of stuff around in the build context which seems to be the suggested way of doing that. COPY can take files that sometimes doesn't exist.
Feels however like this just makes everything more complex and fragile, I think just
setting a new value like
ENV MW_ENABLE_PROFILING_HEADER=${INSTALL_PROFILING_DEPS}
and look that this is the same expected value the install_profiling.sh script expects.
There was a problem hiding this comment.
something like f4ce15e
| // requires docker image to be built with dependencies | ||
| if( isset ( $_SERVER['HTTP_X_WBSTACK_MW_PROFILING'] ) ) { | ||
| $wgProfiler['class'] = 'ProfilerXhprof'; | ||
| $wgProfiler['output'] = [ 'ProfilerOutputText', 'ProfilerOutputDump' ]; |
There was a problem hiding this comment.
This output format really sucks though.
Ideally this could be hooked up to xhgui for the local minikube cluster maybe? Still unclear to me how to do that.
There was a problem hiding this comment.
I think we could still move forward with this PR and have a followup to make it more usable
This allows profiling deps to be installed conditionally in the docker file. It also allows for the X-WBSTACK-MW-PROFILING header to enable it.
toban
force-pushed
the
add-profiling-deps
branch
from
9510cdc
to
e4bb2df
Compare
|
Just resolved a simple conflict. looking at this (and the debug option that is already merged) again it would be nice to not need to include these layers in the docker file that is used for production deployment. |
This allows profiling deps to be installed conditionally in the docker file.
It also allows for the X-WBSTACK-MW-PROFILING header to enable it.