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

feat: Add OpenTelemetry extension to allow auto-instrumentation #784

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

cedricziel
Copy link
Contributor

@cedricziel cedricziel commented Aug 18, 2024

Summary

Add the open telemetry php extension via pecl

Use Cases

Enable observability for PHP apps built with the CNB

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

Closes: #785

Copy link

linux-foundation-easycla bot commented Aug 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@cedricziel cedricziel marked this pull request as ready for review August 18, 2024 11:40
@cedricziel cedricziel requested a review from a team as a code owner August 18, 2024 11:40
@cedricziel
Copy link
Contributor Author

@sophiewigmore - I'm not sure how to set the labels 🤔

@sophiewigmore sophiewigmore added the semver:patch A change requiring a patch version bump label Sep 4, 2024
@sophiewigmore
Copy link
Member

Hi @cedricziel thanks for the PR. This change seems reasonable, however you'll need to add it to the extension lists in the files here as well: https://github.com/paketo-buildpacks/php-dist/tree/f0fcf6a87f5304092f826e192a287951d5ed3684/dependency/actions/compile/extensions-manifests. Make sure that the extension version is compatible with the version of PHP that corresponds to the extension file

@cedricziel
Copy link
Contributor Author

@sophiewigmore done 👍 ext-opentelemetry is compatible with php >= 8 so I added it to all 3 manifests.

@sophiewigmore
Copy link
Member

@cedricziel cool! thanks. can you please edit the PR to point to this new branch I've made? https://github.com/paketo-buildpacks/php-dist/tree/opentelemetry

Since this affects the dependency compilation code, I want to push it to a branch on this repository so we can check it works in our dependency update Github action first. We can do that by triggering it off that branch and seeing all of our tests go green with the resulting dependencies

@sophiewigmore
Copy link
Member

sophiewigmore commented Sep 10, 2024

In thinking about doing that, I just realized we also test the compiled PHP dependency and check the extensions, so you may need to update the tests/fixtures here to include the new extension: https://github.com/paketo-buildpacks/php-dist/tree/95ca8b063cc3511e4dcb319d83dd802ec4997ef8/dependency/test

@cedricziel cedricziel changed the base branch from main to opentelemetry September 12, 2024 19:24
@cedricziel
Copy link
Contributor Author

@sophiewigmore added them to the test apps and retargeted the branch

@sophiewigmore sophiewigmore merged commit 268128f into paketo-buildpacks:opentelemetry Sep 13, 2024
1 check passed
@sophiewigmore
Copy link
Member

@cedricziel cedricziel deleted the patch-1 branch September 14, 2024 20:32
sophiewigmore pushed a commit that referenced this pull request Sep 17, 2024
* feat: Add OpenTelemetry extension to allow auto-instrumentation

* add extension to 2nd manifest

* use pecl recipe

* add ext-opentelemetry to manifests

* Add ext-opentelemetry to test app

---------

Co-authored-by: Sophie Wigmore <[email protected]>
@sophiewigmore
Copy link
Member

@cedricziel re-testing here with an automation fix - https://github.com/paketo-buildpacks/php-dist/actions/runs/10905342607

sophiewigmore pushed a commit that referenced this pull request Sep 17, 2024
* feat: Add OpenTelemetry extension to allow auto-instrumentation (#784)

* feat: Add OpenTelemetry extension to allow auto-instrumentation

* add extension to 2nd manifest

* use pecl recipe

* add ext-opentelemetry to manifests

* Add ext-opentelemetry to test app

---------

Co-authored-by: Sophie Wigmore <[email protected]>

* remove some versions for re-compilation

* Updating buildpack.toml with new versions 8.2.19, 8.3.7, 8.1.28

---------

Co-authored-by: Cedric Ziel <[email protected]>
Co-authored-by: Sophie Wigmore <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should provide ext-opentelemetry to make apps observable with auto-instrumentation
2 participants