[feat] WithOpenTelemetry()

[shyouhei]

OpenTelemetry のトレーサーに対応します。そういったものは大抵プロセスグローバルな設定があり、SDKの外側からなんらかの方法で注入されることと思いますので、そのようなAPIを用意しました。

[repeatedly]

少し内部で話しましたが、profile/envvar等で https://github.com/sacloud/sacloud-otel-collector への設定をしたら、そこへ投げる機能も欲しいかなと思いました。そうすればsaclient-goを利用する各プロダクトでセットアップの重複がなくなります。

別のパッチで対応するのも問題ないかなと思います。

[yamamoto-febc]

otelt対応は是非にというところなのですが、otel SDK + contribへの依存が発生してしまう、かつ今の所これらは結構な頻度で更新されているイメージなので頻繁に取り込みが必要というのがなやましいですね...

saclient-goは低レベルなライブラリで他のSDKから依存される側なのである程度安定して欲しい思いがあります。

iaas-api-goではこの問題への対応としてotel対応については別のgoモジュールにして必要な人だけotel依存部分を使ってもらうようにしていました
https://github.com/sacloud/iaas-api-go/tree/main/trace/otel

salient-goの場合は現状の実装だと別モジュールにするのは厳しいですかね..??
http.Clientまたはhttp.ClientのTransport(RoundTripper)を差し替え可能にする方向に持っていくのもありかもですがちょっと大きな変更になるかもです。

[shyouhei]

プルリクエストをあしがかりに議論が進むのは良いことだと思います！少なくともこのプルリクエストは今のままの状態でいきなりマージはせず、方向性をもんでいきたいです。

現在、主にトレースしたくなっているのは go-retryablehttp の中でクライアントサイドでリトライが発生している場合、ですので、これをcontrib依存ぬきで実現するには、 http.Transport をいわゆるdependency injectionできる必要があるということかなと思います。でも saclient.WithTransport() みたいなやつを作成すればそれで解決するか？というと、Transportって自分で全部書くのすごく大変じゃないですかね…? otelhttp.NewTransport()だって、既存のなにかのTransportはある前提で、それをなんらか弄って、みたいな流れなんだよな。どうするといいんでしょうね。

[yamamoto-febc]

contribを使うこと自体は問題ないと思っています。ただ、現状だと利用者の側でcontribに依存する/しないを選べず常に依存してしまうのでそれはなんとかしたいなと考えています。

saclient.WithTransport()のようなインターフェースを用意することで、汎用的な方法でRoundTripperを差し替えて処理を挟み込めるようになるのがキモと思ってまして、これによりotelに依存する部分を別モジュールにしても外からRoundTripperを差し替えられるようになるのが嬉しいポイントかと思います。RoundTripperはgoの標準ライブラリのnet/httpで提供されている標準的な方法ですし潰しが効きそうです。

また、必ずしもTransportをゼロから実装する必要はないと思います。
我々の側で

• WithTransport的な、差し替え用のインターフェースを用意する
• 別モジュールとしてOTelでトレースを取得するTransportの実装の提供

あたりをしておけばクライアント側はそこまで大変ではないかと思います。

[shyouhei]

https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#NewTransport

func NewTransport(base http.RoundTripper, opts ...Option) *Transport

この base をどうするのかというところが悩ましいです。その下の例は

// Create an http.Client that uses the (ot)http.Transport
// wrapped around the http.DefaultTransport
_ = http.Client{
    Transport: NewTransport(http.DefaultTransport),
}

になっていますけれど、これだと、DefaultTransportしか使えないじゃないですか? なにを言っているかというと、我々がWithTransportを実装したとして、

c1, _ := saclient.Client{}.DupWith(WithTransport(t1))
c2, _ := c1.DupWith(WithTransport(t2), ...)

みたいなことはできない(書いてもc2はt1かt2のどちらかしか動かないようにしか実装できない)ですよねえ。otelhttpとしては本来はそういう、多段階でRoundTripperをネストして使うみたいな用途ではNewTransportの引数によしなに渡してくださいという設計思想だと思うのだけれど。

[yamamoto-febc]

論点は2つですかね?

• http.RoundTripperを誰がどう組み立てるか
• saclient.Clientでhttp.RoundTripperを受け取った時にどうするか (差し替え or chainとして組み立てる)

前者については

otelhttpとしては本来はそういう、多段階でRoundTripperをネストして使うみたいな用途ではNewTransportの引数によしなに渡してくださいという設計思想だと思うのだけれど。

こちらの通り利用者側で組み立ててもらえば良いと思います。http.RoundTripperはインターフェースになっているのは、そこにどのような実装を当てはめるのかを利用者の責務としているからだと思っています。

これで行くと後者も差し替えで良いと思っていまして、c2ではt2を使えばOK、t2でt1とchainさせたいなら予め利用者側でそのようにRoundTripperを組み立ててね、とするのがシンプルそうです。

[yamamoto-febc]

別の論点として仮にWithTransport()を用意した場合、既存の func WithMiddleware(m ...Middleware)とどう棲み分けるかという点もありそうです。この方向で行く場合はhttp.RoundTripperをMiddlewareにラップするような何かを用意するみたいな対応も必要になるかも

[shyouhei]

うーん、「利用者側」が、必ずしもすべての組み合わせをコントロールできるかというと、違うよなあというのがMiddlewareが求められた由来なのですよね。たとえば WithBigInt() は中でMiddlewareを使って実装されていますが、これをRoundTripperを使って同じことをするのは難しい(なぜならWithBigIntの利用者はクライアントが他にどんな機能を求めているか知らないから)だろう、というふうに思います。

もう少し言うと、たとえば WithBigInt() の「利用者」は、SDKなわけですけれども、他の、たとえばotelの「利用者」はSDKを使っている側のusacloud等であることが想定され、そこにズレがあるんですよね。双方を俯瞰的に把握して全部を「利用者」が組み立ててください、というのは、難しいのではないかと思います。誰がやるのか定かでないので

[yamamoto-febc]

利用者がSDKとアプリなど複数の種類があり、最終利用者にRoundTripperの組み立て責務を寄せ切れない、という点は理解しました。

ただ、そのこと自体は「矛盾なく合成しやすい拡張ポイントが必要」という話であって、直ちに middleware を選ぶ理由ではないようにも見えています。
middleware に寄せるべき理由があるとすると、既存の WithXxx() 実装との整合性や、複数機能を独立に積み上げる時の合成規約をライブラリ側で持ちやすい点かなと思っています。

---

この議論は詰めていくべきだとは思うのですが、議論がPRでやりたかったことと少々ずれてきたように思います。
(私が言い出したのに... 申し訳ないです 🙏)

一旦 #76 (review) の議論に戻りたいのですが、

• (1) このまま進めるとotelへの依存が常に発生するが問題ないか?
• (2) 回避したい場合はmiddleware vs RoundTripperあたりの議論が必要そう(合意まで時間がかかるかも?)

あたりを判断する必要がありそうです。

1)について、otel対応はぜひ使いたい、かつ後から依存を消すこともできるはずなので一旦otel依存は許容する、でも良いのではないかと思い始めています。

2)は別Issueにして引き続き議論するというのでいかがでしょうか?

[fujiwara]

SDK で otel trace を扱えるというのは2段階あると思っています。

1. userが作ったtrace情報が入ったctxを渡すと、SDKが発行するHTTPリクエストの traceparent ヘッダに伝播する
2. SDK自体が http client span や独自の span を発行する

1だけやる場合

import "..../saclientotel" // 独立したgo.modをもってotel依存はここに持たせる

var c saclient.Client
err := c.SetWith(saclientotel.WithTracePropagation())

のようにしてやれば、SDKを使う側は明示的にimport+client生成をすることになるので、使わない場合の依存は増えません。暗黙で ctx に入った trace を引き継いでしまうと、ユーザーとしては意図しないtraceの外部漏洩にもなりかねないので、opt-in のためにも必要そうです。

2までやる場合はotel依存が全体に差し込まれることになりますが、その場合も WithSpans() を用意して Propagation とは独立させておくのがよいかと思います。