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

Add APIs for getting the current logfire span #675

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Dec 13, 2024

Addresses #674

This is a proposed implementation for how we can make it possible to get access to the current LogfireSpan, for making use of the various methods/etc. on it that aren't on the result of opentelemetry.trace.get_current_span() when you don't have access to the value.

The motivating use case for this is having access to methods of LogfireSpan inside functions instrumented with @logfire.instrument. Right now, there is no way to get access to the LogfireSpan, as opentelemetry.trace.get_current_span doesn't return that. This PR makes it so that logfire.current_logfire_span() returns the LogfireSpan if the current otel span corresponds to a LogfireSpan, and something API-compatible if it does not. (I also added logfire.current_span() as a more easily discovered re-export of opentelemetry.trace.get_current_span, don't necessarily need to keep that.)


Note that we've discussed making it so that when you call opentelemetry.trace.get_current_span() you get a LogfireSpan back, but the naive approach to this would have the consequence that spans created by other instrumentations would also end up being LogfireSpans, with the associated modified method behaviors/overhead/etc. This may be a good idea, but it's not immediately clear to me.

The good news is that the implementation in this PR would be backwards compatible if we were to make that change, and most of the logic brought in in this PR would just get removed, just the current_span and current_logfire_span functions would remain.


I haven't added any tests yet, mostly because I don't want to put in the effort if the approach is going to be rejected.

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

I think we should use the existing OTEL APIs

Copy link

cloudflare-workers-and-pages bot commented Dec 13, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 27f1424
Status: ✅  Deploy successful!
Preview URL: https://afe037b0.logfire-docs.pages.dev
Branch Preview URL: https://dmontagu-get-current-logfire.logfire-docs.pages.dev

View logs

@dmontagu dmontagu force-pushed the dmontagu/get-current-logfire-span branch from 3f2e6ee to 17cfbc7 Compare December 13, 2024 22:00
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 99.91%. Comparing base (f0b2989) to head (27f1424).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
logfire/current_span.py 69.23% 7 Missing and 1 partial ⚠️
logfire/_internal/main.py 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #675      +/-   ##
===========================================
- Coverage   100.00%   99.91%   -0.09%     
===========================================
  Files          137      138       +1     
  Lines        10750    10793      +43     
  Branches      1473     1475       +2     
===========================================
+ Hits         10750    10784      +34     
- Misses           0        7       +7     
- Partials         0        2       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

I would suggest we add just current_logfire_span, but perhaps for consistency it should be called get_current_logfire_span?

Is there a noticeable memory or CPU overhead? I assume not, but we should perhaps do some basic checks.

Let's see what @alexmojaki says, but I'm strongly in favour of moving quickly, even if it's with an imperfect solution.

Copy link
Member

Choose a reason for hiding this comment

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

are we sure this should be public.

@@ -21,6 +21,7 @@
from ._internal.main import Logfire, LogfireSpan
from ._internal.scrubbing import ScrubbingOptions, ScrubMatch
from ._internal.utils import suppress_instrumentation
from .current_span import current_logfire_span, current_span
Copy link
Member

Choose a reason for hiding this comment

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

I think it could be confusing or cause weird side effects that the imported current_span overrides the current_span module.

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