search

humanmade / aws-xray

HM Platform AWS X-Ray Integration
23 stars 4 forks source link

get_in_progress_trace assumes HTTP context #72

Closed roborourke closed 4 years ago

roborourke commented 4 years ago

The function get_in_progress_trace() assumes that XRay is running in an HTTP context. You can't always rely on things like HTTP_HOST being shimmed in CLI contexts so it should check for the context it's in before adding HTTP related data to the trace.

We'll also need to check anywhere that outputs this data and ensure it can handle the lack of it.

joehoyle commented 4 years ago

Hmm wondering how you triggered this, as we shouldn't be loading X-Ray on the CLI at all?

I think it needs more consideration if we want to add it in that context, as really X-Ray is built around the concept of HTTP requests. We can certainly map WP CLI to it, but we'll want to decide how to track things like the running command, execution status etc.

rmccue commented 4 years ago

See also #8.

roborourke commented 4 years ago

Soz, for clarity it's currently triggered when you run PHPUnit, at least via composer dev-tools phpunit in Altis. I believe it occurs during the install process that the WP PHPUnit bootstrapping spawns.

It's fine to just avoid loading XRay if it's not really suitable for CLI purposes but perhaps there's other contexts we need to consider. This might well be best fixed elsewhere eg. in Altis itself in that case.

joehoyle commented 4 years ago

Ah yes, for now atleast, we should modify Altis Cloud to not load it under php_sapi_name() === 'cli' not just WP_CLI.

roborourke commented 4 years ago

Cheers, will PR that now