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

Make php examples runnable #2896

Merged
merged 13 commits into from
Jun 27, 2023

Conversation

brettmc
Copy link
Contributor

@brettmc brettmc commented Jun 21, 2023

  • Make sure that the examples all run if copy/pasted
  • Update/improve the PHP documentation

Fixes: open-telemetry/opentelemetry-php#1042


Preview: https://deploy-preview-2896--opentelemetry.netlify.app/docs/instrumentation/php/

@brettmc brettmc requested review from a team June 21, 2023 08:32
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

lgtm

ResourceAttributes::SERVICE_NAME => 'bar',
ResourceAttributes::SERVICE_VERSION => '0.1',
ResourceAttributes::DEPLOYMENT_ENVIRONMENT => 'development',
])));
$transport = (new GrpcTransportFactory())->create('http://collector:4317' . OtlpUtil::method(Signals::TRACE));
Copy link
Member

Choose a reason for hiding this comment

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

I get an error on this line:

PHP Fatal error:  Uncaught Error: Class "OpenTelemetry\Contrib\Grpc\GrpcTransportFactory" not found in /Users/tnajaryan/work/experiments/otel-php/GettingStarted.php:31
Stack trace:
#0 {main}
  thrown in /Users/tnajaryan/work/experiments/otel-php/GettingStarted.php on line 31

Do I need to install the Contrib somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the example to not use grpc (it's covered better in the exporters doc).

Copy link
Member

Choose a reason for hiding this comment

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

Tried the updated example, now it can't find SpanExporter:

PHP Fatal error:  Uncaught Error: Class "OpenTelemetry\Contrib\Otlp\SpanExporter" not found in /Users/tnajaryan/work/experiments/otel-php/GettingStarted.php:32
Stack trace:
#0 {main}
  thrown in /Users/tnajaryan/work/experiments/otel-php/GettingStarted.php on line 32

@tigrannajaryan
Copy link
Member

Please also make examples in Logging section runnable.

I tried this example. It does not include the Use directives and won't work.

After copying the Use directives from the first example I got:

PHP Fatal error:  Uncaught Error: Class "ConsoleExporter" not found in /Users/tnajaryan/work/experiments/otel-php/GettingStarted.php:28
Stack trace:
#0 {main}
  thrown in /Users/tnajaryan/work/experiments/otel-php/GettingStarted.php on line 28

I think it needs to use

I also tried example from the repo. It works but requires changing line

require __DIR__ . '/../../vendor/autoload.php';

to

require 'vendor/autoload.php';

@brettmc
Copy link
Contributor Author

brettmc commented Jun 23, 2023

@tigrannajaryan more updates made to the docs. I've made it more clear which code sections are a continuation of the initial example, and made the others self-contained.

brettmc added 12 commits June 27, 2023 16:04
- Make sure that the examples all run if copy/pasted.
Update/improve the PHP documentation
simplify manual instrumentation example by removing
grpc (which is covered in the exporters section)
add a log processor and exporter.
several issues have been filed where it was not clear that the
extension wasn't a complete solution (like most APM products provide).
to keep the manual examples succinct, let's just export them to console.
exporters are covered in more detail in their own page.
@chalin chalin force-pushed the make-php-examples-runnable branch from 7950f22 to ddaebd4 Compare June 27, 2023 20:04
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Overall there seem to be some good updates in here, thanks!

My main objection is to the addition of manual instrumentation in Getting started. For one, the getting-started page is meant to allow a reader to get some telemetry up-and-running quickly: and the automatic instrumentation does that nicely. Also, we're trying to be consistent across languages: do most other language SIGs add manual instrumentation instructions in their Getting started page @svrnm?

Comment on lines +493 to +494
- Learn more about [manual instrumentation][] and try out some
[examples](/docs/instrumentation/php/examples/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice update here, and good catch.

@cartermp
Copy link
Contributor

Python and .NET have a small section, but Ruby and Java do not. So, uhh, it's mixed 😆

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Ok, then: @svrnm, @cartermp et al., if you're ok with the added "manual instrumentation" to the Getting started, then so am I.

Overall, the rest LGTM.

Thanks for the updates @brettmc!

@cartermp cartermp merged commit 7fa1b14 into open-telemetry:main Jun 27, 2023
@brettmc brettmc deleted the make-php-examples-runnable branch July 7, 2023 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot make getting started manual instrumentation example to work
5 participants