Trace http response code when set via function 'http_response_code'

[aderiyenko]

Description

fixes #750

Readiness checklist

• (only for Members) Changelog has been added to the appropriate release draft. Create one if necessary.
• Tests added for this feature/bug.

Reviewer checklist

• Appropriate labels assigned.
• Milestone is set.
• Changelog has been added to the appropriate release draft. For community contributors the reviewer is in charge of this task.

[labbati]

Hi @aderiyenko thanks so much for the contribution. Let us take a look into it asap and see why tests are failing! We will be able to help with that part.

[aderiyenko]

Hi @labbati , thanks ! As I understand this change (adding a callback for http_response_code new functionality) requires all the integrations tests for app supported frameworks/CMSes/PHP versions combinations. In fact, there will be much more integrations tests code than the couple of line change in the library, as I understand.

Few questions here.

1. Is my understanding correct that I need to add two base tests to tests/Frameworks/TestScenarios.php as I did in this PR and then implement them for all the supported libraries?
2. If yes, I would need to setup my local environment for this. Rather than pushing to remote and triggering CircleCI on every change like I was initially hoping to to ( I did not realise there were so manu supported frameworks - sorry!)
3. see my issue Contributing.md might be missing steps #756 . I need to update it I think. These might be some valid points, but they are not blockers for me at the moment. I'll update the issue on what's blocking me right now and appreciate your team helping me with setting up my local env for this.

Please confirm if my understanding of the testing approach is correct (p.1)

[barker2424]

Hi @labbati , any update from your end? Thanks!

[labbati]

looking into this right now

[aderiyenko]

Hi @labbati , I updated #756 with some info yesterday, so I basically had to switch to 5.6 and 7.0 containers and start with their suites. And for 5.6 I also pushed some changes to docker-compose (see changes here) related to the context of the 5.6 dockerfile building. LMK if it makes sense.

Right now I'm continuing writing integration tests for 5.6 and 7.0 . I'd appreciate if you can confirm my approach in p.1 in the comment above.

Thanks!

[labbati]

Hi @aderiyenko, no need to add tests for ALL the combinations. In this case it is enough to add one integration test for a generic web request. I can do it adding a commit to your PR if you like, you were already very kind contributed the functionality.

In case you want to do it, you can add one specific test method to tests/Integration/BootstrapTest.php as it is there that you register the callback.

My apologies for having looked at your PR sooner.

[aderiyenko]

so just to make sure:

1. I need to remove the two required tests I added to tests/Frameworks/TestScenarios.php
2. I can leave the ones I already added for slim3, laravel4, cake2.8, CI2.2 and some symfony versions?
3. if I remove it from p.1 it stops being required in all the integrations test combinations?

[labbati]

Here is what I would do:

1. I would revert all the changes to tests/Frameworks
2. I would revert all changes to tests/Integrations
3. (let me change this part to make it easier, otherwise you have to change more things) I would add one test case tests/Integration/ResponseStatusCodeTest.php that is pretty much copied from tests/Integration/ResponseStatusCodeTest.php with some small changes.
   a. in ResponseStatusCodeTest_files/index.php I would just do something like http_response_code(456);
   b. The test then it would just be something like:
   1. $traces = $this->tracesFromWebRequest(function () { $this->call(GetSpec::create('Root', '/')); });
   2. Verify the tag exists and is 456: something like
   $this->assertExpectedSpans( $traces, [ SpanAssertion::build( 'web.request', 'web.request', 'web', 'web.request' )->withExactTags([ 'http.method' => 'GET', 'http.url' => '/simple', 'http.status_code' => '456', 'integration.name' => 'web', ])->withExactMetrics([ '_dd1.sr.eausr' => 0.3, '_sampling_priority_v1' => 1, ]), ] );

The reason why I am proposing this is that this is not something specific to a web framework specifically, so it should not be part of every web framework test.

We use web frameworks test to test internal spans that are very specific of every framework.

In this case we only need to test this functionality as a generic one.

Does it make sense?

[aderiyenko]

@labbati do you know why this https://github.com/DataDog/dd-trace-php/pull/755/files#diff-6faa5b95536965feb0d26b3065c9430aR182 is not marking the span as error? It doesn't get https://github.com/DataDog/dd-trace-php/pull/755/files#diff-6faa5b95536965feb0d26b3065c9430aR182 here when I test with xdebug locally and therefore not here: https://github.com/DataDog/dd-trace-php/blob/master/src/DDTrace/Span.php#L177

[labbati]

Hi @aderiyenko and thanks for the fantastic work so far.
Here you find the final steps to get it merged.
I hope I listed the steps clearly enough to be easy to follow.

[labbati]

Also, I updated the contributing guides: #769
You can find an up to date docs here: https://github.com/DataDog/dd-trace-php/blob/ebdcec6ebcd4add9cf64fe033051d0f13c39602b/CONTRIBUTING.md

[aderiyenko]

Also, I updated the contributing guides: #769
You can find an up to date docs here: https://github.com/DataDog/dd-trace-php/blob/ebdcec6ebcd4add9cf64fe033051d0f13c39602b/CONTRIBUTING.md

Thanks! I think it helps a lot anyone who wants to contribute

[aderiyenko]

seeing a segfault in the pipeline and not sure how to restart it without any extra commits

[aderiyenko]

@morrisonlevi will add now, just saw your comment, huge thanks!

[aderiyenko]

Thanks @morrisonlevi and @labbati ! When do you think 0.41 will be released?

[labbati]

Great work @aderiyenko, merging this PR right now.

[labbati]

When do you think 0.41 will be released?

early next week

[aderiyenko]

sounds great, thanks again for all your help with this!