-
Notifications
You must be signed in to change notification settings - Fork 211
feat: Add experimental response propagator interface #1712
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
feat: Add experimental response propagator interface #1712
Conversation
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1712 +/- ##
============================================
- Coverage 68.37% 68.10% -0.28%
- Complexity 2878 2919 +41
============================================
Files 430 435 +5
Lines 8757 8876 +119
============================================
+ Hits 5988 6045 +57
- Misses 2769 2831 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This looks pretty sensible. Can you add an example of how it's used? |
$sdkBuilder->setPropagator($propagator); | ||
|
||
$responsePropagators = []; | ||
foreach ($properties['response_propagator']['composite'] as $plugin) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I agree that ideally this is how it would work, and the config spec allows extra entries here, but I think we should be cautious about adding non-spec entries here. At the least, it should be experimental_response_propagator
. My main concern is incompatibility if/when something is added to spec.
The config repo may accept a PR adding it as experimental and optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Changed to use response_propagator/development
This looks pretty good. Most of my review comments are about not over-committing or doing things that might conflict with future spec changes. We need to be cautious while moving ahead of the spec. |
Co-authored-by: Brett McBride <[email protected]>
Co-authored-by: Brett McBride <[email protected]>
Yes, it is quite a lot of files change if adding the interface to |
Co-authored-by: Tobias Bachert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go - just one minor tidy up thing please
Issue
Currently there are 2 response propagators (ServerTimingPropagator & TraceResponsePropagator) in otel-php to propagate information to response. However, they are called explicitly in different instrumentations (e.g. Slim, Symfony, etc). As a result, vendors are unable to propagate their specific information to response.
Proposal
With the ongoing otel spec PR (1355, 3825) and they are still under review, I am proposing an experimental response propagator interface so as to allow vendor to propagate their specific response.
Once this PR is in, both ServerTimingPropagator and TraceResponsePropagator can implement the same ResponsePropagatorInterface and the following code in the instrumentation library could be replaced
by