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

update quoteservice to opentelemetry-php beta #644

Merged
merged 7 commits into from
Dec 21, 2022

Conversation

brettmc
Copy link
Contributor

@brettmc brettmc commented Dec 19, 2022

Changes

  • update to opentelemetry-php beta versions
  • import only required opentelemetry-php libraries
  • use php auto-instrumentation (remove most manual instrumentation, leaving enough to illustrate that auto+manual can work together)
  • bump php to 8.2, the most recent version supported by opentelemetry-php

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs folder

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

- update to opentelemetry-php beta versions
- import only required opentelemetry-php libraries
- use php auto-instrumentation (remove most manual instrumentation, but leave some manual instrumentation to illustrate that auto+manual can work together)
- bump php to 8.2, the most recent version supported by opentelemetry-php
@brettmc brettmc requested a review from a team December 19, 2022 00:05
Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

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

Please update the docs/servcies/quoteservice.md docs and create a changelog entry.

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

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

Thank you for this!

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

This looks great @brettmc, thanks for that!

I've tested it locally and it seems that we have some issues though.
After building this quoteservice I've started getting the following:

quoteservice             | [Mon Dec 19 06:26:48.124913 2022] [php:notice] [pid 19] [client 172.18.0.16:57740] Slim Application Error\nType: Error\nCode: 0\nMessage: Class "OpenTelemetry\\API\\Trace\\Span" not found\nFile: /var/www/app/routes.php\nLine: 41\nTrace: #0 [internal function]: {closure}(Object(Slim\\Psr7\\Request), Object(Slim\\Psr7\\Response))\n#1 /var/www/vendor/php-di/invoker/src/Invoker.php(74): call_user_func_array(Object(Closure), Array)\n#2 /var/www/vendor/php-di/slim-bridge/src/ControllerInvoker.php(47): Invoker\\Invoker->call(Object(Closure), Array)\n#3 /var/www/vendor/slim/slim/Slim/Routing/Route.php(384): DI\\Bridge\\Slim\\ControllerInvoker->__invoke(Object(Closure), Object(Slim\\Psr7\\Request), Object(Slim\\Psr7\\Response), Array)\n#4 /var/www/vendor/slim/slim/Slim/MiddlewareDispatcher.php(81): Slim\\Routing\\Route->handle(Object(Slim\\Psr7\\Request))\n#5 /var/www/vendor/slim/slim/Slim/MiddlewareDispatcher.php(81): Slim\\MiddlewareDispatcher->handle(Object(Slim\\Psr7\\Request))\n#6 /var/www/vendor/slim/slim/Slim/Routing/Route.php(341): Slim\\MiddlewareDispatcher->handle(Object(Slim\\Psr7\\Request))\n#7 /var/www/vendor/slim/slim/Slim/Routing/RouteRunner.php(84): Slim\\Routing\\Route->run(Object(Slim\\Psr7\\Request))\n#8 /var/www/vendor/slim/slim/Slim/Middleware/RoutingMiddleware.php(59): Slim\\Routing\\RouteRunner->handle(Object(Slim\\Psr7\\Request))\n#9 /var/www/vendor/slim/slim/Slim/MiddlewareDispatcher.php(147): Slim\\Middleware\\RoutingMiddleware->process(Object(Slim\\Psr7\\Request), Object(Slim\\Routing\\RouteRunner))\n#10 /var/www/vendor/slim/slim/Slim/Middleware/BodyParsingMiddleware.php(68): Psr\\Http\\Server\\RequestHandlerInterface@anonymous->handle(Object(Slim\\Psr7\\Request))\n#11 /var/www/vendor/slim/slim/Slim/MiddlewareDispatcher.php(147): Slim\\Middleware\\BodyParsingMiddleware->process(Object(Slim\\Psr7\\Request), Object(Psr\\Http\\Server\\RequestHandlerInterface@anonymous))\n#12 /var/www/vendor/slim/slim/Slim/Middleware/ErrorMiddleware.php(107): Psr\\Http\\Server\\RequestHandlerInterface@anonymous->handle(Object(Slim\\Psr7\\Request))\n#13 /var/www/vendor/slim/slim/Slim/MiddlewareDispatcher.php(147): Slim\\Middleware\\ErrorMiddleware->process(Object(Slim\\Psr7\\Request), Object(Psr\\Http\\Server\\RequestHandlerInterface@anonymous))\n#14 /var/www/vendor/slim/slim/Slim/MiddlewareDispatcher.php(81): Psr\\Http\\Server\\RequestHandlerInterface@anonymous->handle(Object(Slim\\Psr7\\Request))\n#15 /var/www/vendor/slim/slim/Slim/App.php(215): Slim\\MiddlewareDispatcher->handle(Object(Slim\\Psr7\\Request))\n#16 /var/www/vendor/slim/slim/Slim/App.php(199): Slim\\App->handle(Object(Slim\\Psr7\\Request))\n#17 /var/www/public/index.php(56): Slim\\App->run()\n#18 {main}
quoteservice             | 172.18.0.16 - - [19/Dec/2022:06:26:48 +0000] "POST /getquote HTTP/1.1" 500 3612 "-" "-"

Could you take a look?

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

I've built it in my Ubuntu and it worked.
Not sure what happened on Windows, but it LGTM!

@julianocosta89
Copy link
Member

Just adding a screenshot here for visibility:
Screenshot from 2022-12-19 20-07-56

With this change quoteservice produces 14 spans per request.

@cartersocha
Copy link
Contributor

Lgtm. Merging.

Thanks @brettmc for the contribution! 🦞

@cartersocha
Copy link
Contributor

And @bobstrecansky for taking time to help with the review 😎

@cartersocha cartersocha merged commit c839dd7 into open-telemetry:main Dec 21, 2022
@puckpuck
Copy link
Contributor

took me a while to test this one out, but I also see the same as @julianocosta89 did. The quoteservice now produces several auto-instrumented spans, well beyond what should be expected.

@brettmc can you please take a look at this? I'm going to open an issue as well.
Screen Shot 2022-12-24 at 12 40 24 AM

@puckpuck puckpuck added the helm-update-required Requires an update to the Helm chart when released label Jan 17, 2023
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants