feat: move OTEL to a new 'InstrumentedFosite' #763
Conversation
|
Tests seem to just be failing due ory/hydra using a PR in go.mod. I like this change, I did some experimentation with it in making github.com/ory/fosite/otel it's own module so it's entirely standalone however I could not get it working, maybe it's unnecessary due to pruning and github.com/ory/x contains the imports anyway. |
There was a problem hiding this comment.
Thank you for the PR! I think it's nice to abstract away tracing and at the same time am critical of this approach. We will most likely add more OTEL tracing to lower-level functions and then this approach will no longer work (as well) without significant code wiring.
I think otel is a solid standard and would recommend to just go with it for now. If you want wrapping - you still can do that in your own codebase consuming Fosite!
|
The way we abstracted this for future proofing is to introduce a metrics interface that allows you to swap in providers as needed. OTEL is great but is not completely adopted by major vendors (referenced in my post and from actual usage of said vendor SDK and providers). Also the current implementation takes away any flexibility for adopters to use different libraries. I am happy to take another stab at this to put it in Fosite directly if that will be acceptable. With regards to adopters wrapping Fosite, the current implementation doesn't let me do that because OTEL is used directly without an opt-out |
|
@aeneasr Just got to my machine. To substantiate further, we could minimally add a config parameter to control the instrumentation. I was also thinking for handlers to add a wrapper handler that basically does the same thing and it becomes a simple matter of adding that to your compose factory/handler list. For custom code blocks, it is simple enough to add an interface for spans and transactions. |
|
I don't think that we want to maintain an abstraction for telemetry in Fosite, especially as we are quite happy with otel at the moment. If you don't have an otel compatible system, you can set the otel tracer to noop/nil in which case nothing happens. And you can still wrap Fosite any way you want to add instrumentation that works with vendor XYZ. Being honest, I don't see the value of doing a lot of work here right now. |
The recent change in [#761 ] introduces OTEL as part of Fosite. While this is valuable, this should
be provided as a wrapper and not as a hard wired part of Fosite, which is meant to be a SDK. Also,
popular instrumentation options and APM vendors have some level of support for OTEL but shine
when their wrapper SDKs are used that do not always comply with OTEL interfaces. See New Relic
and Instana SDKs for reference.
This change proposes that OTEL should be an opt-in as part of a wrapper implementation of Fosite
that is called
InstrumentedFositeunder theotelpackage. It also offers a pattern that can be appliedto any code blocks and handlers that should be instrumented to derive maximum benefits from a telemetry
perspective. For example - there may be no value in instrumenting a code block that interfaces with no
external components beyond the overall telemetry offered at a HTTP middleware level, whereas there
may be a great deal of value instrumenting call outs to external APIs and databases.
Related Issue or Design Document
#761
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
Further comments