Cleaning up the examples a little #741
Conversation
…in the example with
Codecov Report
@@ Coverage Diff @@
## main #741 +/- ##
============================================
+ Coverage 82.49% 82.86% +0.36%
- Complexity 1263 1295 +32
============================================
Files 145 150 +5
Lines 3125 3192 +67
============================================
+ Hits 2578 2645 +67
Misses 547 547
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Grunet
requested review from
bobstrecansky,
zsistla,
tidal and
brettmc
as code owners
examples/traces/demo/src/index.php
Outdated
| @@ -1,7 +1,7 @@ | |||
| <?php | |||
|
|
|||
| declare(strict_types=1); | |||
| require 'vendor/autoload.php'; | |||
| require '/../../../../vendor/autoload.php'; | |||
There was a problem hiding this comment.
This doesn't work, since the main libraries autoloader is out of the scope of the docker-compose context, and it wouldn't pull in the depencies of the demo (slim, etc.), so it needs to be require 'vendor/autoload.php';
There was a problem hiding this comment.
Yup good call, switched it back.
I also was curious and tried to get this demo working, but got stuck after updating the tags on the opentelemetry-php-base images ("latest" was pulling in an image from 7 months ago) and exposing the 4317 port on the collector.
Have a hunch something's off with the exporting from PHP but couldn't figure out how to troubleshoot (since this example wants to do all the config via environment variables and the OTEL_LOG_LEVEL one isn't supported yet afaict)
There was a problem hiding this comment.
Yap, I tried the demo as well a few days ago, and it didn't really work. However I used it about 2 months ago for a Otel workshop at work, and it worked fine.
Maybe @brettmc can have a look at it, once he is back. (Can you Brett?)
There was a problem hiding this comment.
I sure can take a look at it, right now.
There was a problem hiding this comment.
@Grunet @tidal I found a regression/bug. It's kinda hard to write a test for, since it involves a TracerProvider being destroyed as soon as it is created (I think introduced in the fix for #716 )
In short, this works:
$tracerProvider = (new TracerProviderFactory('example'))->create();
$tracer = $tracerProvider->getTracer('io.opentelemetry.contrib.php');and this doesn't work:
$tracer = (new TracerProviderFactory('example'))->create()->getTracer('io.opentelemetry.contrib.php');
//or
$tracer = (new TracerProvider([$spanProcessor])->getTracer('foo');I'll take this up on slack...
|
@Grunet Thanks!!! There is only one small issue, which needs to be fixed, otherwise I'd merge this PR as is, as it already improves the examples by a lot. |
|
I also added in a make target to just try and run through more of the examples to make sure nothing crashes (I also found a bug along the way using it! Another incorrect path issue). Figured it might be a helpful sanity check in the future w/o a super high maintenance burden (if there's a clever way not to have to list individual example files out too that'd be cool to learn about too) |
If you would like to work on that, you could add a command to https://github.com/opentelemetry-php/dev-tools. For finding the respective file, one could use FilesystemIterator or Symfony's Finder Component. To run the tests in an isolated process one could use Symfony's Process Component Let me know, if that's somthing you would be interested in. Other than that, I'll merge this PR. :) |
Oh yeah interesting I hadn't thought of actually using PHP to manage that! (I was just trying to think a little if there was some clear 1 liner makefile thing, or shell script thing, but gave up pretty quick) I think it's probably ok for now imo (seems safe/quick to refactor if it gets unwieldy). I'll go ahead and take a look at what else I can maybe help with amongst the issues |
Resolves #426
I still need to smoke test the examples (and probably set up a shortcut to do that more easily in the future, as much as is feasible), butmostly looking for initial feedback on if the new folder structure seems improved to folks.Otherwise I mostly tried to incorporate the thoughts I gathered from other folks in the issue thread.