ReactPHP HTTP Browser auto-instrumentation #368
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. |
|
I guess the first question is, can this be (or could it be in the future) an auto-instrumentation for more parts of react? i.e., the framework/ecosystem, rather than just this component? Are there other components that could be useful to hook? |
|
Two things are missing that I can see:
|
Absolutely. The other half of the HTTP library is a server, so that’s an obvious choice for what to instrument next. There are underlying socket and stream interfaces that might be beneficial to instrument for non-HTTP use-cases along with UDP and DNS interfaces, although it would be redundant if you instrumented all of them. At a higher level the async fibers could be auto instrumented, although I’m not sure how useful that would be as it is more in the weeds of how an application is built, and auto-instrumentation is more for the “edges”. |
Updated |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
============================================
+ Coverage 81.60% 82.05% +0.44%
- Complexity 1722 1771 +49
============================================
Files 133 136 +3
Lines 7269 7461 +192
============================================
+ Hits 5932 6122 +190
- Misses 1337 1339 +2 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Hmm. Edit: figured out how to use |
Cool, that makes sense. I just wanted to check whether we shouldn't be naming this "React" to allow for other components, but the current approach seems sensible. |
I debated between the current name and "ReactPHP"; I don't think I really have a preference. @WyriHaximus Maybe that naming discrepancy would be a candidate for deptrac? |
Okay, hopefully I got it right this time. Can we try the tests again? |
|
Okay can we try once more please? |
|
Woohoo! The tests for this component finally passed. |
src/Instrumentation/ReactHTTP/examples/http+async_nonblocking.php
Outdated
Show resolved
Hide resolved
The HTTP component is the most high-level one, the rest are mainly building blocks for it and other context aware protocols. I'm also maintaining https://github.com/jakubkulhan/bunny/ and would love to see/do an autoinstrumentation for it. There is https://github.com/friends-of-reactphp/mysql and https://github.com/voryx/PgAsync on a database level, and https://github.com/clue/reactphp-redis/ for redis. I'm unsure if we should all put them into one component or split them across several. I'm also reading up on all of this, how it works etc. One thing I need to figure out is how it all plays with (nested) fibers.
With ReactJS becoming known as react we wanted to emphasise that we're a different project so we always put the PHP behind it.
Yes, yes please. Having a look at the code as well now, will add some comments. Haven't fully caught up on everything so I might be asking obvious questions. |
| hook( | ||
| Transaction::class, | ||
| 'send', |
There was a problem hiding this comment.
This hook call is for the extension to intercept and do its magic, right?
There was a problem hiding this comment.
that's right, it registers the pre/post callbacks via the zend observer API, so that they are executed when that function is called.
There was a problem hiding this comment.
Is this the preferred way, even for maintainers, to implement things? Because I do like that this doesn't add any code to projects directly and keeps the instrumentation isolated from a package's logic.
There was a problem hiding this comment.
@WyriHaximus from https://opentelemetry.io/docs/specs/otel/overview/#instrumentation-libraries
The inspiration of the project is to make every library and application observable out of the box by having them call OpenTelemetry API directly. However, many libraries will not have such integration, and as such there is a need for a separate library
I'm not a maintainer of any of the repos discussed here, so just my opinion, but from what I've read in the OpenTelemetry docs, the preferred approach for library maintainers would be to instrument their library's code directly. They would have more control over the signals generated, performance would be better, and it would be maintained by those who know how to maintain it the best. Beyond instrumenting core PHP extensions and PSRs, it feels like the auto-instrumentation libraries in this repo are more of a kickstart until OpenTelemetry is more widely adopted. (And since I don't control the ReactPHP code, this was a way for me to get the instrumentation I needed—even if OpenTelemetry rejected my PR, I could change the namespace and use it in our projects internally. That said, I also use Christian's Redis package, so I'll probably end up writing something for that too... maybe for that I'll see if he's open to a PR to add the instrumentation directly. 🤷 )
I will update the naming from Edit: This has been changed.
I have not used the server side; perhaps this is something @WyriHaximus you would consider adding once you're more familiar with OpenTelemetry? |
Edit: I found coverage for everything except the |
|
Should the examples move to https://github.com/open-telemetry/opentelemetry-php-contrib/tree/main/examples/instrumentation? |
IMHO lets focus on getting this in first, then look at the server part, and then figure out the rest. If @clue isn't open to it, I'm down to set it up through another package, whether that is here, or I host it. The same goes for the other databases I mentioned. I have no problem slapping another package at it.
<3 👍
I'm down for that. Let me start with Bunny in one of my projects, which also does HTTP calls, so I can test the work here and get familiar with OT's systems. And figure out all the things 👍 . Thanks again for doing this @smaddock, it make me look at something I've been wanted to look at for ages but never got around doing 😅 . |
|
This is hopefully getting pretty close. Thank you @brettmc @Nevay and @WyriHaximus for working with me on this lengthy PR discussion and for all the feedback; I really appreciate it! Questions I still have:
Once this is approved, if the Curl, Guzzle, and HttpAsyncClient libraries don't have specific maintainers, I'd kind of like to update those with some of the same changes y'all recommended here. |
I think where they are is fine, it makes sense for them to be with the associated code. The original examples dir was from back when we had a small number of packages.
Do you mean the two dot points about when the span should start&finish? Can you paste-quote in the bits that concern you?
Automatically, i'll tag and release an initial version once it's merged
You should add
I don't think so. gitsplit entry is the most important, and you've done that. I'll let you test |
Specifically:
and also:
@Nevay brought up earlier that the behavior of the spans differs based on how the Browser is called. I documented the differences in |
github-project-automation
bot
moved this from In Progress
to Done
in PHP Prioritized Backlog
|
@brettmc I'm using the Guzzle library here: Open to other suggestions though! |
4 participants
First attempt at auto-instrumentation for the client side of https://reactphp.org/http/
Tried to copy the structure and functionality I saw implemented here for Guzzle and HTTPlug, but let me know if I missed something, or those are out of date, the name needs to change, or you need anything else.
Had one of the ReactPHP core maintainers (@WyriHaximus) help iron out some kinks.
Examples are included for a few different ways of implementing the ReactPHP HTTP Browser.
Integration test covers 93% of the actual auto-instrumentation file (everything specific to ReactPHP.)
PHP_VERSION=8.3 make allis passing on this PR.