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 TraceResponse propagator #143

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Conversation

cedricziel
Copy link
Contributor

@cedricziel cedricziel commented Apr 7, 2023

Note: This introduces a new experimental package as traceresponse is currently a w3c editors' draft.

This package provides a Trace Context HTTP Response Headers Format
propagator to inject the current span context into Response datastructures.

The main goal is to allow client-side technology (Real User Monitoring, HTTP Clients) to record
the server side context in order to allow referencing or assuming it.

@cedricziel cedricziel requested a review from a team April 7, 2023 11:08
@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #143 (526fc48) into main (f897e94) will increase coverage by 0.85%.
The diff coverage is 100.00%.

❗ Current head 526fc48 differs from pull request most recent head 24632e7. Consider uploading reports for the commit 24632e7 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #143      +/-   ##
============================================
+ Coverage     25.03%   25.88%   +0.85%     
- Complexity      315      319       +4     
============================================
  Files            32       33       +1     
  Lines          1306     1321      +15     
============================================
+ Hits            327      342      +15     
  Misses          979      979              
Flag Coverage Δ
7.4 73.02% <100.00%> (+0.97%) ⬆️
8.0 30.28% <100.00%> (+0.94%) ⬆️
8.1 30.37% <100.00%> (+0.94%) ⬆️
8.2 25.90% <100.00%> (+0.85%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...tion/TraceResponse/src/TraceResponsePropagator.php 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f897e94...24632e7. Read the comment docs.

@brettmc
Copy link
Collaborator

brettmc commented Apr 7, 2023

A couple of small things from me, but this looks good.

We might need to think about how this propagator interacts with the tracecontext one; for example we current can globally configure a propagator which is then accessible from API\Globals::propagator(). The spec talks about a composite propagator - we don't have one yet (probably because it has not been needed before now)

@cedricziel
Copy link
Contributor Author

We might need to think about how this propagator interacts with the tracecontext one; for example we current can globally configure a propagator which is then accessible from API\Globals::propagator(). The spec talks about a composite propagator - we don't have one yet (probably because it has not been needed before now)

True - the reason i didnt opt for this, is that traceresponse is strictly limited to responses and without a specific setter implementation, the propagator wont know.

My first approach was to explore in core to have Globals::responsePropagators(), but that seemed to be too invasive for an experimental package.

@cedricziel cedricziel requested a review from brettmc April 7, 2023 13:25
Cleanup dependencies

Add PHPStan config and clean code

Allow PHP 7 compatible symfony/http-client

Add TraceResponse to .gitsplit

Fix test method casing
@cedricziel
Copy link
Contributor Author

Note that I separated out Symfony (#146) and Slim (#147) auto propagation so we can discuss them separately after this is merged, gitsplit happened and the package has been created.

@brettmc brettmc merged commit ba372ed into open-telemetry:main Apr 11, 2023
@cedricziel cedricziel deleted the trace-response branch April 11, 2023 08:25
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