Revamp examples

[Blacksmoke16]

As a result of #422, examples will need to be updated to work with the new API. A lot of them are also very similar. IMO it would be better to have the examples show off specific features, like a simple one, child spans, etc. Instead of every type of exporter getting its own example.

Originally posted by @Blacksmoke16 in #422 (comment)

[tidal]

In addition it would be good to use snake case for the file names of the examples, as camel case is usually only used for classes, interfaces and traits.

[brettmc]

Hi. I have an example that I've put together whilst trying to work out the latest API changes (which I am exporting to elastic APM to check that it presents as expected). I'll clean it up and submit it for consideration.

[brettmc]

It seems to me that most of the original examples are really developer-focussed integration/regression tests - any thoughts on moving them out of examples and into regression or something like that? (happy to do this, and get them working again, if they're still useful and required).

[Blacksmoke16]

There's also an Integration namespaces in tests/SDK that could be a good place for some of these types of examples.

I still think the examples would be better served showing how to use higher level concepts of the library (e.g. simple versus parent/child) instead needing one for every tiny feature of the lib. Possibly this would also be a good place for textual docs that can just talk about those features versus needing an entirely new example file that's 95% the same, just with a diff exporter/extra config/data on the spans.

[lalex]

Could we rework or totally remove examples named AlwaysOn... as they show bad pattern of exposing Samplers implementation? Client's code should not rely on sampling decisions made by samplers.

I've noticed it while browsing the source of the Symfony bundle example: https://github.com/tidal/otel-sdk-bundle-example-sf5/blob/6b84645171d7f4fac06c82ec857291fb58ffecad/src/EventSubscriber/OtelKernelSubscriber.php#L193

[tidal]

Could we rework or totally remove examples named AlwaysOn... as they show bad pattern of exposing Samplers implementation? Client's code should not rely on sampling decisions made by samplers.

I've noticed it while browsing the source of the Symfony bundle: https://github.com/tidal/otel-sdk-bundle-example-sf5/blob/6b84645171d7f4fac06c82ec857291fb58ffecad/src/EventSubscriber/OtelKernelSubscriber.php#L193

That's not the source code of the Symfony Bundle. However I'm excited somebody found my easter egg, since I actually forgot about it,
I agree, that the sampling decisions should not be in the examples. They are more confusing, than helpful anyway.
One simple example for each sampler should be enough.

The sampling decisions are also used in the Symfony/Laravel tutorials, but what I'm more concerned of there is that the shown way of "integrating" via global scope is rather dangerous imo.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Grunet]

Is there anything concrete left to do on this issue? I noticed just now looking at them altogether they're a tad disorganized (I guess at least at the file level) and as a result it's a little hard to tell what's there or not.

Would renaming the files per @tidal's suggestion and then organizing them into folders/sub-folders be enough? Or do folks think more is needed around this area before the beta release?

[tidal]

@Grunet

Would renaming the files per @tidal's suggestion and then organizing them into folders/sub-folders be enough? Or do folks think more is needed around this area before the beta release?

That would definitly be a great start (there's probably always room for improvement). Also the examples should be organized per signal (trace, logs, metrics)
However some examples might be outdated or broken.
Especially we should get rid of all:

if (SamplingResult::RECORD_AND_SAMPLE === $samplingResult->getDecision()) {
[...]

and try-catch blocks
(as we are not leaking exceptions and must not according to the spec)

try {
    $span1 = $tracer->spanBuilder('foo')->startSpan();
    $span1Scope = $span1->activate();

    try {
        $span2 = $tracer->spanBuilder('bar')->startSpan();
        echo 'OpenTelemetry welcomes PHP' . PHP_EOL;
        $span2->end();
    } finally {
        $span1Scope->detach();
        $span1->end();
    }
} catch (Throwable $t) {
    $rootSpan->recordException($t);
} finally {
    //ensure span ends and scope is detached
    $rootScope->detach();
    $rootSpan->end();
}

[MurzNN]

which I am exporting to elastic APM to check that it presents as expected

@brettmc Could you please share also the exporter configuration for APM? I've configured "otlp/http" and it doesn't work with Elastic APM collector, but works well with Grafana Tempo collector. And I still can't find any examples of how to make it work, so an example would be very useful!

[brettmc]

Great question, @MurzNN
I'm afraid I didn't end up publishing my example anywhere, so you'd need to consult Elastic's documentation. I plan on picking up again my efforts to export open telemetry to Elastic APM in the coming weeks (for my day job), so I can report back when I do.

I'm not sure if it is still a requirement, but at the time only gRPC was supported by elastic (which was a problem for us since we didn't have http/2 enabled). The workaround at the time was to put an opentelemetry collector on the elastic host which could accept http/protobuf and send on as grpc. Hope this helps!

[MurzNN]

@brettmc Did you got any good news about this?

[brettmc]

I haven't started yet, and won't for at least a few weeks sorry.