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.

https://opentelemetry.io/

It has a PHP implementation: https://opentelemetry.io/docs/instrumentation/php/

The rationale for using OpenTelemetry is at #638078: Automated performance testing for core.

  1. 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-http

    php-http/discovery runs a composer plugin to discover PSR-18 implementations.

  2. 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.

  3. 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/security

    Code 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

Comments

catch created an issue. See original summary.

catch’s picture

Status: Active » Needs review
StatusFileSize
new31.34 KB

.

catch’s picture

Issue summary: View changes

Added additional dependencies that are pulled in by opentelemetry to the issue summary, but haven't done the evaluations for those yet.

catch’s picture

StatusFileSize
new31.82 KB

Had no idea the dictionary scanned composer files..

catch’s picture

Issue summary: View changes

Added additional dependency evaluations for the dependencies of the dependency.

catch’s picture

StatusFileSize
new31.92 KB
catch’s picture

Issue summary: View changes
catch’s picture

StatusFileSize
new31.92 KB

Status: Needs review » Needs work

The last submitted patch, 8: 3352389-6.patch, failed testing. View results

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

 

catch’s picture

Status: Needs work » Needs review
StatusFileSize
new31.93 KB

Should fix both test failures.

smustgrave’s picture

Feel 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 ?

heddn’s picture

Looking 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.

catch’s picture

Issue summary: View changes
catch’s picture

StatusFileSize
new31.58 KB

Re-rolled and updated to beta.

borisson_’s picture

Status: Needs review » Needs work

Need reroll

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Status: Needs work » Needs review
StatusFileSize
new31.75 KB

Re-rolled.

smustgrave’s picture

Status: Needs review » Needs work

For the CC.

catch’s picture

Status: Needs work » Needs review
StatusFileSize
new32.32 KB

Added to the dictionary.

heddn’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, 21: 3352389.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
StatusFileSize
new32.32 KB

Fixing the metapackage.

Status: Needs review » Needs work

The last submitted patch, 24: 3352389.patch, failed testing. View results

andypost’s picture

Would be great to explore upcoming compatibility with PHP 8.3, ATM https://pecl.php.net/package/opentelemetry works fine

catch’s picture

Status: Needs work » Needs review

HEAD 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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

All green!

Think a follow up should be opened for #26 too.

longwave’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release manager review +10.2.0 release notes

I 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!

  • longwave committed faef0c76 on 11.x
    Issue #3352389 by catch, smustgrave, xjm, heddn: Add open-telemetry/sdk...
wim leers’s picture

Issue tags: +Performance

I had no idea this was happening! 🤩🤯

Exciting to see where this goes next!

catch’s picture

Status: Fixed » Active

This broke subtree splits due to allow-plugins, I'll make a patch.

catch’s picture

Status: Active » Needs review
StatusFileSize
new1.28 KB

Maybe this.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

  • longwave committed 1b06a823 on 11.x
    Issue #3352389 followup by catch: Add open-telemetry/sdk and open-...
longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed #34.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture