Slim and psr-15 auto-instrumentation #81
Conversation
Co-authored-by: Tobias Bachert <[email protected]>
…lemetry-php-contrib into slim-auto-instrumentation
src/Instrumentation/Psr15/README.md
Outdated
| <?php | ||
| require_once 'vendor/autoload.php'; | ||
|
|
||
| $tracerProvider = <create tracer provider>; |
There was a problem hiding this comment.
Do we plan to simplify that in production ready release?. Important aspect of auto-instrumentation is easy of use. Using this approach user needs to know some details of opentelemetry, at least how to create trace provider and export spans, then he has to add opentelemetry dependencies to application. This might be good for experienced user that want to know what's going on under the hood, but 99% users IMHO will be not interested in such details.
There was a problem hiding this comment.
If all inputs are provided via environment variables, we could automatically create and register everything. Combining https://github.com/open-telemetry/opentelemetry-php/blob/main/examples/traces/features/configuration_from_environment.php with the composer autoload-files technique is one way I think we could provide zero-code instrumentation.
There was a problem hiding this comment.
That's what I was thinking about
There was a problem hiding this comment.
With open-telemetry/opentelemetry-php#849 we could define lazy initializers in the SDK that are invoked iff the user does not configure the instances manually.
Globals::registerInitializer(static function(Configurator $configurator): Configurator {
$tracerProvider = (new TracerProviderFactory('should be fetched from resource instead'))->create();
ShutdownHandler::register($tracerProvider->shutdown(...));
return $configurator->withTracerProvider($tracerProvider);
});There was a problem hiding this comment.
Just to clarify, how will we check if user configured these instances, should it be part of Globals::registerInitializer implementation?
There was a problem hiding this comment.
I think Globals already takes care of that - eg if you try to get a tracer provider, it will first check in context:
public static function tracerProvider(): TracerProviderInterface
{
return Context::getCurrent()->get(ContextKeys::tracerProvider()) ?? self::globals()->tracerProvider;
}
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
============================================
- Coverage 89.58% 80.60% -8.99%
- Complexity 283 330 +47
============================================
Files 31 34 +3
Lines 816 1000 +184
============================================
+ Hits 731 806 +75
- Misses 85 194 +109
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
src/Instrumentation/Slim/tests/Unit81/CallableFormatterTest.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Tobias Bachert <[email protected]>
Co-authored-by: Tobias Bachert <[email protected]>
add class to span name
| $span->recordException($exception, [TraceAttributes::EXCEPTION_ESCAPED => true]); | ||
| $span->setStatus(StatusCode::STATUS_ERROR, $exception->getMessage()); | ||
| } | ||
| if ($response?->getStatusCode() >= 500) { |
There was a problem hiding this comment.
Should capture some additional http attributes such as http.status_code.
There was a problem hiding this comment.
done, added a few more attributes.
suggested change from Nevay Co-authored-by: Tobias Bachert <[email protected]>
adding more http trace attributes
| hook( | ||
| RequestHandlerInterface::class, | ||
| 'handle', | ||
| pre: static function (RequestHandlerInterface $handler, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) { |
There was a problem hiding this comment.
I was thinking about current implementation for some time and I don't think that relying on external library parameters (I'm taking about request and its usage to store span) is good thing. I believe that instrumentation should be independent and rely only on itself implementation, therefore why not use Context (I got the same effect with it). Below pre hook that uses additional $rootspan variable
pre: static function (RequestHandlerInterface $handler, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation, $rootspan) {
$request = ($params[0] instanceof ServerRequestInterface) ? $params[0] : null;
$root = Context::getCurrent()->get($rootspan);
$builder = $instrumentation->tracer()->spanBuilder(
$root
? sprintf('%s::%s', $class, $function)
: sprintf('PSR HTTP %s %s', $request?->getMethod(), $request->getUri() ?? 'unknown')
)
->setSpanKind(SpanKind::KIND_SERVER)
->setAttribute('code.function', $function)
->setAttribute('code.namespace', $class)
->setAttribute('code.filepath', $filename)
->setAttribute('code.lineno', $lineno);
$parent = Context::getCurrent();
if (!$root && $request) {
//create http root span
$parent = Globals::propagator()->extract($request->getHeaders());
$parent = $parent->with($rootspan, true);
$span = $builder
->setParent($parent)
->setAttribute(TraceAttributes::HTTP_URL, $request->getUri()->__toString())
->setAttribute(TraceAttributes::HTTP_METHOD, $request->getMethod())
->setAttribute(TraceAttributes::HTTP_REQUEST_CONTENT_LENGTH, $request->getHeaderLine('Content-Length'))
->setAttribute(TraceAttributes::HTTP_SCHEME, $request->getUri()->getScheme())
->startSpan();
} else {
$span = $builder->startSpan();
}
Context::storage()->attach($span->storeInContext($parent));
return [$request];
},
There was a problem hiding this comment.
In your example, is $rootSpan something like a ContextKey(SpanInterface::class)? It seems like a good idea to me.
There was a problem hiding this comment.
Actually, in my example I stored just bool, but I think it does not matter what will be there, main point is to get rid of using request attribute
| RoutingMiddleware::class, | ||
| 'performRouting', | ||
| pre: null, | ||
| post: static function (RoutingMiddleware $middleware, array $params, ServerRequestInterface $request, ?Throwable $exception) { |
There was a problem hiding this comment.
This hook is used only to update span name created by other hook. I think that hooks should be self contained and orthogonal. I would remove this hook and use request->getUri() in handle hook(s)
There was a problem hiding this comment.
The reason I've done it as "set" then later "update" is:
- the spec says that span names by default should not be the uri: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#name (too-high cardinality)
- slim doesn't have a better route name until after the routing middleware has run (which it might not do, if an error occurs earlier)
There was a problem hiding this comment.
Maybe, we can extract part of uri, it would simplify code.
| * Create a span representing the http transaction for Slim\App::handle | ||
| * @psalm-suppress ArgumentTypeCoercion | ||
| */ | ||
| hook( |
There was a problem hiding this comment.
Actually, I don't understand need for this hook as we already hooking into RequestHandlerInterface::handle, is it only due to ending root span?
There was a problem hiding this comment.
You're right, it's redundant now. I left it there because I wasn't sure which way I'd end up going, but if having Psr15 instrumentation work out when to create a root span in RequestHandlerInterface::handle, then this hook can be removed.
get rid of using request attributes to store parent information and use context instead
|
@brettmc I'm thinking about merging this PR as it is now. We can make improvements later with additional PRs |
|
@pdelewski great idea - I haven't even looked at this PR in a week. I'll give it a quick tidy up and submit, then others can collaborate |
No description provided.