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.

Sign up for GitHub

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

Jump to bottom

[quoteservice] Replace php-cli to php-apache #563

Merged
merged 8 commits into from
Nov 11, 2022
Merged

[quoteservice] Replace php-cli to php-apache #563

merged 8 commits into from
Nov 11, 2022

Conversation

julianocosta89
Copy link
Member

@julianocosta89 julianocosta89 commented Nov 8, 2022 •
edited
Loading

Changes

PHP-CLI is a PHP built-in server and should not be used in production, more info here: https://www.php.net/manual/en/features.commandline.webserver.php#features.commandline.webserver.
In order to have a service that users can relate to real life services, I've updated the QuoteService to make use of PHP-Apache.

I've also added OPcache in order to improve the PHP performance: https://www.php.net/manual/en/intro.opcache.php

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes

@julianocosta89 julianocosta89 requested a review from a team November 8, 2022 09:05
Juliano Costa added 2 commits November 8, 2022 10:10
@mviitane
Copy link
Member

mviitane commented Nov 8, 2022

Seems to be running fine. One comment though. Is it possible to fix the two first lines from the log:

$ docker compose logs quoteservice
quoteservice  | AH00558: apache2: Could not reliably determine the server's fully qualified domain name, using 172.18.0.5. Set the 'ServerName' directive globally to suppress this message
quoteservice  | AH00558: apache2: Could not reliably determine the server's fully qualified domain name, using 172.18.0.5. Set the 'ServerName' directive globally to suppress this message
quoteservice  | [Tue Nov 08 09:12:53.126646 2022] [mpm_prefork:notice] [pid 8] AH00163: Apache/2.4.54 (Debian) PHP/8.1.12 configured -- resuming normal operations
quoteservice  | [Tue Nov 08 09:12:53.127943 2022] [core:notice] [pid 8] AH00094: Command line: 'apache2 -D FOREGROUND'
quoteservice  | 172.18.0.15 - - [08/Nov/2022:09:13:10 +0000] "POST /getquote HTTP/1.1" 200 167 "-" "-"
quoteservice  | 172.18.0.15 - - [08/Nov/2022:09:13:14 +0000] "POST /getquote HTTP/1.1" 200 184 "-" "-"
quoteservice  | 172.18.0.15 - - [08/Nov/2022:09:13:14 +0000] "POST /getquote HTTP/1.1" 200 169 "-" "-"

@julianocosta89
Copy link
Member Author

Seems to be running fine. One comment though. Is it possible to fix the two first lines from the log:

$ docker compose logs quoteservice
quoteservice  | AH00558: apache2: Could not reliably determine the server's fully qualified domain name, using 172.18.0.5. Set the 'ServerName' directive globally to suppress this message
quoteservice  | AH00558: apache2: Could not reliably determine the server's fully qualified domain name, using 172.18.0.5. Set the 'ServerName' directive globally to suppress this message
quoteservice  | [Tue Nov 08 09:12:53.126646 2022] [mpm_prefork:notice] [pid 8] AH00163: Apache/2.4.54 (Debian) PHP/8.1.12 configured -- resuming normal operations
quoteservice  | [Tue Nov 08 09:12:53.127943 2022] [core:notice] [pid 8] AH00094: Command line: 'apache2 -D FOREGROUND'
quoteservice  | 172.18.0.15 - - [08/Nov/2022:09:13:10 +0000] "POST /getquote HTTP/1.1" 200 167 "-" "-"
quoteservice  | 172.18.0.15 - - [08/Nov/2022:09:13:14 +0000] "POST /getquote HTTP/1.1" 200 184 "-" "-"
quoteservice  | 172.18.0.15 - - [08/Nov/2022:09:13:14 +0000] "POST /getquote HTTP/1.1" 200 169 "-" "-"

Good catch @mviitane, fixed!

mviitane
mviitane approved these changes Nov 8, 2022
View reviewed changes
Copy link
Member

@mviitane mviitane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Also the logs were fixed.

@julianocosta89 julianocosta89 changed the title Replace php-cli to php-apache [quoteservice] Replace php-cli to php-apache Nov 8, 2022
@julianocosta89
Copy link
Member Author

julianocosta89 commented Nov 9, 2022 •
edited
Loading

@mviitane I've also added OPCache in order to improve the PHP performance (more added to the initial changes text).
Do you mind just re-validating it?

@julianocosta89
Copy link
Member Author

@bobstrecansky would you be able to have a look?

@julianocosta89 julianocosta89 self-assigned this Nov 10, 2022
@julianocosta89 julianocosta89 added the enhancement New feature or request label Nov 10, 2022
@mviitane
Copy link
Member

@mviitane I've also added OPCache in order to improve the PHP performance (more added to the initial changes text).
Do you mind just re-validating it?

Seems to be running fine, didn't notice any functional changes.

@julianocosta89
Copy link
Member Author

@mviitane I've also added OPCache in order to improve the PHP performance (more added to the initial changes text).
Do you mind just re-validating it?

Seems to be running fine, didn't notice any functional changes.

No functional change, just better performance.

@puckpuck puckpuck merged commit 856f4fb into open-telemetry:main Nov 11, 2022
@julianocosta89 julianocosta89 deleted the quoteservice-php-apache branch November 11, 2022 06:08
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
@cartersocha @puckpuck
[quoteservice] Replace php-cli to php-apache (open-telemetry#563)
e6744db 
* Replace php-cli to php-apache

* lint

* Changelog

* Add ServerName

* Add OPCache for better performance

Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
Co-authored-by: Pierre Tessier <pierre@pierretessier.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mviitane mviitane mviitane approved these changes

Assignees

@julianocosta89 julianocosta89

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@julianocosta89 @mviitane @puckpuck @cartersocha