Conversation
|
@aeneasr @alnr The intent here is great but I wanted to suggest that we use a different approach here. Rather than adding this code directly into the Handlers, could we not instead create a wrapper handler - say OTELInstrumentedTokenHandler - that takes a reference to the actual handler, then performs the instrumentation in the implemented This also provides a pattern for other types of instrumentation libraries - using New Relic, Instana etc. - which offer capabilities beyond OTEL to integrate into their respective platforms. |
|
I agree. I am fairly certain this specific intention could also be accomplished leveraging the decorator pattern. The individual functions and handlers which require this functionality could easily be extended using this pattern to avoid adding this dependency to fosite entirely. It is possible there is a specific reason this is not viable that I've missed, however. |
|
I am happy to port these changes into a decorator/wrapper handler that can be used. That would preserve what you are doing here. One other motivation here is to allow for other instrumentation libraries without needing changes in actual handlers. Now, for specific code blocks that might need to be instrumented - it might make sense to add a more generic metrics interface. It doesn't look like you are doing that here but it might be an expansion to this to instrument "expensive" or critical code blocks like the process of creating a token session, which usually involves a data tier component that isn't MemoryStore. |
Bumped dependencies and Go to 1.20 and fixed a ton of lint issues.