Problem/Motivation
Spin-off from #3346765: Add PerformanceTestBase for allowing browser performance assertions within FunctionalJavaScriptTests which is itself a spin-off from #638078: Automated performance testing for core.
Open telemetry is a collection of libraries/projects to support performance instrumentation in a variety of languages.
It has a PHP implementation: https://opentelemetry.io/docs/instrumentation/php/
The rationale for using OpenTelemetry is at #638078: Automated performance testing for core.
-
OpenTelemetry Dependency Evaluation
Maintainership of the package
The project appears well-maintained and has multiple official maintainers as well as active contributors, details at https://github.com/open-telemetry/opentelemetry-php/blob/main/CONTRIBUTI...
It's part of the broader OpenTelemetry project, although I'm not clear how that works organisationally.
OpenTelemetry itself is under the umbrella of the Cloud Native Computing Foundation https://www.cncf.io/
Security policies of the package
Prefer e-mail, github issue as a last resort, documented here: https://github.com/open-telemetry/opentelemetry-php/security/policy
Expected release and support cycles
Approaching a 1.0.0 release (currently beta), the 0.x series had tagged releases approximately once per month. Uses semver for release numbering.
Code quality
Modern PHP, strict types, decent online and in-code documentation.
Other dependencies it would add, if any (the full tree, not just direct dependencies), and evaluations for those dependencies as well
OpenTelemetry PHP also brings in:
php-http/plug
This in turn brings in:
php-http/guzzle-adapter
php-http/discovery
php-http/promise
All under https://github.com/php-httpphp-http/discovery runs a composer plugin to discover PSR-18 implementations.
-
Dependency evaluation for php-http/plug
Maintainership
Four active maintainers from different organisations: https://github.com/orgs/php-http/people
Security policy
https://docs.php-http.org/en/latest/development/contributing.html - issues reported privately via e-mail.
Releases
Appear to be approximately once per year, only minor version releases since 2018, semver.
Code quality
Modern PHP, semver, well documented. Not very much code between these three, it's all glue so that projects can require an http client without specifying which one.
-
Dependency evaluation for google/protobuf
Maintainership
Maintained by google.
https://github.com/protocolbuffers/protobuf-php
this is a subtree split from the main multi-language repository:
https://github.com/protocolbuffers/protobuf/Release cycle
Approximately quarterly releases. The previous minor release is maintained for at least a year after a new major release. https://protobuf.dev/support/version-support/
Security support
Contributions require a CLA
Security policy (google's private reporting process): https://github.com/protocolbuffers/protobuf/securityCode quality
Modern PHP, well documented.
Proposed resolution
Add opentelemetry as a development dependency.
Note that OpenTelemetry requires a telemetry collector to receive traces, however the current implementation of #3346765: Add PerformanceTestBase for allowing browser performance assertions within FunctionalJavaScriptTests skips this logic if no endpoint is defined.
Tag1 is working with the Drupal Association to provide an endpoint, however it would be useful to have the dependency in core while that's being worked on, and it's not a hard dependency for the new performance tests which run and do their own assertions without it.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | 3352389-allow-plugins.patch | 1.28 KB | catch |
| #24 | 3352389.patch | 32.32 KB | catch |
Comments
Comment #2
catch.
Comment #3
catchAdded additional dependencies that are pulled in by opentelemetry to the issue summary, but haven't done the evaluations for those yet.
Comment #4
catchHad no idea the dictionary scanned composer files..
Comment #5
catchAdded additional dependency evaluations for the dependencies of the dependency.
Comment #6
catchComment #7
catchComment #8
catchComment #10
xjmComment #11
xjmComment #12
catchShould fix both test failures.
Comment #13
smustgrave commentedFeel like this may be a dumb question but since this is a dev dependency is there any harm in using the beta tag of https://github.com/opentelemetry-php/exporter-otlp ?
Comment #14
heddnLooking at the release notes in https://github.com/opentelemetry-php/exporter-otlp/releases/tag/1.0.0beta5, I like the suggestion of moving to the beta tag. And we should work on getting the pecl extension added to the test runner One thing I've noticed from running this locally is that it feels like the instrumentation seems to add lot of overhead. The release notes seem to bear that out. But switching to the C library should help.
Comment #15
catchComment #16
catchRe-rolled and updated to beta.
Comment #17
borisson_Need reroll
Comment #19
catchRe-rolled.
Comment #20
smustgrave commentedFor the CC.
Comment #21
catchAdded to the dictionary.
Comment #22
heddnI was a little concerned about the implications of
php-http/guzzle7-adapter, but it seems to be fairly well maintained and someday might not even be needed as it appears to be a shim. +1 on these dev-dependency changes.Comment #24
catchFixing the metapackage.
Comment #26
andypostWould be great to explore upcoming compatibility with PHP 8.3, ATM https://pecl.php.net/package/opentelemetry works fine
Comment #27
catchHEAD was broken, resetting status.
I haven't yet tried the opentelemetry pecl package, but yeah it might be useful, although we'd need to add it to drupalci/gitlabci environments to be able to use it.
Comment #28
smustgrave commentedAll green!
Think a follow up should be opened for #26 too.
Comment #29
catchOpened #3367216: consider using the OpenTelemetry pecl package for core performance instrumentation.
Comment #30
longwaveI keep opening this one and hemming and hawing over it; but at the end of the day it's only a set of dev dependencies and we can just as easily remove them again, and now 10.1.0 is out the door I feel more confident in getting this into 10.2.0 early.
Committed and pushed faef0c760d to 11.x (10.2.x). Thanks!
Comment #32
wim leersI had no idea this was happening! 🤩🤯
Exciting to see where this goes next!
Comment #33
catchThis broke subtree splits due to allow-plugins, I'll make a patch.
Comment #34
catchMaybe this.
Comment #35
longwaveFix looks correct to me, same issue as #3294205: [Packaging broken!] Composer v2.2 prompts to authorize another plugin when stability=dev
@catch also opened #3368696: Write a test to ensure that composer plugins match in composer templates to prevent this happening again.
Comment #37
longwaveCommitted #34.
Comment #39
quietone commented