Skip to content

Feat/guzzle traceparent middleware#10

Open
kaz29 wants to merge 5 commits into
mainfrom
feat/guzzle-traceparent-middleware
Open

Feat/guzzle traceparent middleware#10
kaz29 wants to merge 5 commits into
mainfrom
feat/guzzle-traceparent-middleware

Conversation

@kaz29
Copy link
Copy Markdown
Owner

@kaz29 kaz29 commented Jun 1, 2026

No description provided.

kaz29 and others added 2 commits June 1, 2026 18:07
…t propagation

Provides TraceContext\GuzzleMiddleware and GuzzleClientFactory under
src/Middleware/TraceContext/ so applications can inject the current
OpenTelemetry trace context into outbound Guzzle requests via DI.
Optional createSpan flag creates a KIND_CLIENT span per request.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace pecl with pie (PHP Installer for Extensions) in Dockerfiles
and add ext-grpc to both Docker and CI environments.
Remove markTestSkipped guards that were guarding against missing
ext-opentelemetry, as the extension is now reliably available.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kaz29 kaz29 self-assigned this Jun 1, 2026
@kaz29 kaz29 added the enhancement New feature or request label Jun 1, 2026
Copy link
Copy Markdown
Owner Author

@kaz29 kaz29 left a comment

Choose a reason for hiding this comment

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

コードレビュー指摘事項


[バグ] createSpan=true 時のスパンコンテキスト順序(GuzzleMiddleware.php

traceparent ヘッダーは __invoke() 内でスパン起動よりに注入されています。そのため createSpan: true の場合、下流サービスが受け取る traceparent には新しい CLIENT スパンではなく親スパンのスパン ID が含まれてしまいます。

修正案:withSpan() 内でヘッダー注入もまとめて行い、スパン起動後に実施する。

private function withSpan(RequestInterface $request, array $options, callable $handler): PromiseInterface
{
    $method = strtoupper($request->getMethod());
    $uri = $request->getUri();

    $span = Globals::tracerProvider()
        ->getTracer('otel-instrumentation.cakephp.guzzle')
        ->spanBuilder($method . ' ' . $uri->getHost())
        ->setSpanKind(SpanKind::KIND_CLIENT)
        // ... attributes ...
        ->startSpan();

    $scope = $span->storeInContext(Context::getCurrent())->activate();

    // スパン起動後にヘッダーを注入することで、CLIENT スパンの span ID が伝播される
    $headers = [];
    TraceContextPropagator::getInstance()->inject($headers);
    foreach ($headers as $name => $value) {
        $request = $request->withHeader($name, $value);
    }

    return $handler($request, $options)->then(...);
}

合わせて __invoke() 内のヘッダー注入は createSpan: false の場合のみ実行するよう分岐させると整理しやすいです。


[仕様違反] HTTP 4xx に STATUS_OK を設定(GuzzleMiddleware.php L67)

$span->setStatus(StatusCode::STATUS_OK);

OTel HTTP クライアント規約 では:

  • HTTP 5xx → STATUS_ERROR
  • HTTP 4xx → STATUS_UNSET(明示的に OK にしない)
  • HTTP 1xx〜3xx → STATUS_UNSET

修正案:

$statusCode = $response->getStatusCode();
$span->setAttribute('http.response.status_code', $statusCode);
if ($statusCode >= 500) {
    $span->setStatus(StatusCode::STATUS_ERROR);
} // それ以外は STATUS_UNSET のまま(明示的な setStatus 不要)

[軽微] handler キーの上書きが無警告(GuzzleClientFactory.php

ドキュメントに「handler は含めないこと」と明記されていますが、渡しても array_merge でサイレントに上書きされます。カスタムハンドラーが意図せず破棄されるリスクがあります。

public static function create(array $clientConfig = [], bool $createSpan = false): Client
{
    if (isset($clientConfig['handler'])) {
        throw new \InvalidArgumentException("'handler' key is managed by GuzzleClientFactory. Use \$clientConfig without 'handler'.");
    }
    // ...
}

[軽微] getConfig() が Guzzle 7 で非推奨(GuzzleClientFactoryTest.php

$client->getConfig('handler');  // 非推奨
$client->getConfig('base_uri'); // 非推奨

Guzzle 7 で getConfig() は非推奨となっており、Guzzle 8 では削除される可能性があります。ハンドラースタックの確認は HandlerStack::__toString() を直接使う方法で代替できます。

// 例:ミドルウェア登録の確認
$stack = HandlerStack::create();
$stack->push(new GuzzleMiddleware(), 'traceparent');
$this->assertStringContainsString('traceparent', (string) $stack);

kaz29 and others added 3 commits June 2, 2026 11:20
- Fix traceparent propagation order: inject headers after span activation in
  withSpan() so the CLIENT span's span_id is propagated, not the parent's
- Fix HTTP status code handling per OTel semconv: only set STATUS_ERROR for
  5xx responses; leave STATUS_UNSET for all others (no explicit STATUS_OK)
- GuzzleClientFactory now throws InvalidArgumentException when 'handler' key
  is provided in $clientConfig to prevent silent overwrites
- Replace deprecated Guzzle 7 getConfig() calls in tests with HandlerStack
  direct inspection; add tests for server error status and span_id propagation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Allows a custom fixed span name to be specified instead of the default
"METHOD hostname" format, making traces easier to identify in backends.
Falls back to the auto-generated name when not specified.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant