Skip to content
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

feat: basic tracing instrumentation with OpenTelemetry #761

Merged
merged 2 commits into from
Aug 4, 2023
Merged

Conversation

alnr
Copy link
Contributor

@alnr alnr commented Aug 3, 2023

Bumped dependencies and Go to 1.20 and fixed a ton of lint issues.

@alnr alnr marked this pull request as ready for review August 3, 2023 16:04
@alnr alnr requested a review from aeneasr as a code owner August 3, 2023 16:04
@alnr alnr self-assigned this Aug 3, 2023
@aeneasr aeneasr merged commit 198f913 into master Aug 4, 2023
8 checks passed
@aeneasr aeneasr deleted the tracing branch August 4, 2023 07:55
@vivshankar
Copy link
Contributor

vivshankar commented Aug 4, 2023

@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 HandleTokenEndpointRequest type functions? It achieves the same goal without adding a tight dependency.

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.

@james-d-elliott
Copy link
Contributor

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.

@vivshankar
Copy link
Contributor

vivshankar commented Aug 4, 2023

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.

shipperizer pushed a commit to shipperizer/fosite that referenced this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants