-
Notifications
You must be signed in to change notification settings - Fork 10
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
Draft: feat: add trace handling #114
base: main
Are you sure you want to change the base?
Conversation
src/Builder/DefaultBuilderTrait.php
Outdated
@@ -210,17 +215,38 @@ private function convertToMultipartItems(string $key, array|string|\Stringable|i | |||
|
|||
public function generate(): GotenbergFileResult | |||
{ | |||
$this->logger?->debug('Processing file using {sensiolabs_gotenberg.builder} builder.', [ | |||
$trace = ($this->traceGenerator ?? self::defaultTraceGenerator(...))(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to have this fallback early on maybe ? In construct or something. It would avoid to create multiple reference to default generator
$this->logger?->debug('Processing file using {sensiolabs_gotenberg.builder} builder.', [ | ||
$this->traceGenerator ??= $this::defaultTraceGenerator(...); | ||
$trace = ($this->traceGenerator)(); | ||
$headers = ['Gotenberg-Trace' => $trace]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trace should be optional to allow keeping default behavior (generated UUID).
We can make the trace nullable and call it only if set. If so, we should add an "unset" method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder. Wouldn't it be better to always set this ourselves ?
* | ||
* @param \Closure(): string $traceGenerator | ||
*/ | ||
public function traceGenerator(\Closure $traceGenerator): static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function traceGenerator(\Closure $traceGenerator): static | |
public function trace(?\Closure $traceGenerator = null): static |
If set, use the \Closure, if not, use a fn(): string => bin2hex(random_bytes(16)).microtime(true);
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ function noTrace(): static
to remove this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the value. There will be a trace no matter what. Either explicitly set from our component or automaticcaly create by gotenberg. But for webhooks we do need this feature. Withotu trace, webhook won't be possible. Trace act as a token per request.
Also I think noTrace
is misleading. There will be a trace. just not an epxlicit one. So I think null is not needed or act as what you intended for noTrace
==> fallback to default behaviour
Description :
Gotenberg allow overiding it's internal trace https://gotenberg.dev/docs/routes#request-tracing. This PR adds the possiblity to set our own trace to allow debuging and tracing requests.
As suggested by @Neirda24 , the trace could also hold a ressource URI with a unique hash as a query parameter, or anything that could be useful to the user.
This will especially be useful to trace the asynchronous generation webhook calls because the request made by gotenberg to the webhook endpoint will hold the trace in its headers.
TODO: