-
Notifications
You must be signed in to change notification settings - Fork 14
WIP: add otel baggage php endpoints #6213
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
base: main
Are you sure you want to change the base?
Conversation
|
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3593b7566d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RewriteRule "^/otel_drop_in_baggage_api_otel$" "/otel_drop_in_baggage_api_otel/" | ||
| RewriteRule "^/otel_drop_in_baggage_api_datadog$" "/otel_drop_in_baggage_api_datadog/" |
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.
Add PHP handler for /otel_drop_in_baggage_api_datadog
The rewrite enables the /otel_drop_in_baggage_api_datadog route, but a repo-wide search shows there is no corresponding PHP handler under utils/build/docker/php/common/ (only otel_drop_in_baggage_api_otel.php was added). That means the route will 404 in the PHP weblog, so Test_Baggage_Headers_Api_Datadog will fail once it’s enabled. Please add the PHP handler or keep the test skipped for PHP until it exists.
Useful? React with 👍 / 👎.
| <?php | ||
|
|
||
| use OpenTelemetry\API\Baggage\Baggage; |
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.
Require Composer autoload before using OTel API
otel_drop_in_baggage_api_otel.php uses OpenTelemetry\API\Baggage\Baggage but never includes vendor/autoload.php. In this repo, other PHP endpoints that use Composer deps explicitly require the autoloader (e.g., log-library.php), so this endpoint will fatally error with “Class not found” and return 500. Add the autoload include at the top of the file.
Useful? React with 👍 / 👎.
Motivation
Changes
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is present