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

Add support for injecting into an array #23

Merged
merged 1 commit into from
Mar 13, 2020
Merged

Add support for injecting into an array #23

merged 1 commit into from
Mar 13, 2020

Conversation

eddycharly
Copy link
Contributor

Would you be ok to support injecting context into an array instead of Psr\Http\Message\RequestInterface ?

It would allow for easier integration into existing code.

Thanks.

Copy link
Member

@cawolf cawolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, adding this method could help integrating pre PSR-7 applications. I added some comments to the pull request.

Service/Tracing.php Outdated Show resolved Hide resolved
Service/Tracing.php Outdated Show resolved Hide resolved
Service/Tracing.php Outdated Show resolved Hide resolved
Service/TracingService.php Outdated Show resolved Hide resolved
Service/TracingService.php Outdated Show resolved Hide resolved
Service/TracingService.php Outdated Show resolved Hide resolved
@eddycharly
Copy link
Contributor Author

Thanks, I will address your comments today.
I wanted to hear back from before getting further.

@eddycharly eddycharly changed the title [WIP] Add support for injecting into an array Add support for injecting into an array Mar 10, 2020
@eddycharly eddycharly requested a review from cawolf March 10, 2020 09:49
@eddycharly
Copy link
Contributor Author

@cawolf it should be ready for review now

Copy link
Member

@cawolf cawolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the unresolved conversation regarding the interface.

@eddycharly
Copy link
Contributor Author

I'm not sure about this in the MockTracer, shouldn't inject preserve the existing headers ?

public function inject(SpanContext $spanContext, $format, &$carrier)
{
$carrier = ['made_up_header' => '1:2:3:4'];
}

At least, it is what jukylin/jaeger-php seems to be doing
https://github.com/jukylin/jaeger-php/blob/39b3e83e0fe214692253da5318dffea828d41be5/src/Jaeger/Propagator/JaegerPropagator.php#L23-L30

What do you think ? Should i change the MockTracer implementation ? It would avoid a loop in the injectTracingHeadersIntoCarrier method.

@eddycharly eddycharly requested a review from cawolf March 11, 2020 08:10
@cawolf
Copy link
Member

cawolf commented Mar 11, 2020

Sure, you can try to make it work that way. I remember running into issues testing that behaviour exactly because of passing arrays by reference and not be return, that's why that implementation is just pretending to inject headers. After all, it's just here to make the tests work.

@eddycharly
Copy link
Contributor Author

Preserving existing headers in the MockTracer works as expected and doesn't break any existing test.
I guess a tracer implementation not preserving existing headers should be considered a bug in the tracer itself.

Service/TracingService.php Outdated Show resolved Hide resolved
@eddycharly eddycharly requested a review from cawolf March 12, 2020 08:07
@eddycharly
Copy link
Contributor Author

@cawolf any chance to get this merged soon ?

@cawolf cawolf merged commit 10417e6 into auxmoney:master Mar 13, 2020
@eddycharly
Copy link
Contributor Author

thanks !

@cawolf
Copy link
Member

cawolf commented Mar 13, 2020

released in v0.4.2

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.

2 participants