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

Use OPEN_SPANS and custom record_exception logic for all spans #696

Merged
merged 3 commits into from
Dec 20, 2024

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Dec 18, 2024

I'm working toward splitting LogfireSpan into a dedicated context manager, and a subclass of ReadableSpan.

This PR makes some of the changes we want to make along the way. In particular:

  • I ensure all spans created by the logfire _ProxyTracer are tracked in OPEN_SPANS, and therefore should get properly closed on exit (even if created in generators, etc.), not just LogfireSpans
  • I ensure we use our custom logic for record_exception for all spans created by the logfire _ProxyTracer
    • To do this, I needed to move _record_exception and _set_exception_status to tracer to prevent circular imports. They are now used in multiple modules, so I removed the leading _, but they are only used in _internal modules so I think that's okay.
    • As an implementation detail, I modified _MaybeDeterministicTimestampSpan to do the OPEN_SPANS manipulation and override record_exception, since we were always returning spans wrapped with that class from _ProxyTracer.start_span. Given its expanded usage, I also renamed that class to _LogfireWrappedSpan. Given there are now behaviors we want to always override, I think we are paying less marginal overhead for setting the timestamp deterministically, so I think it's fine to rename the class and not even aspire to only use it when modifying the timestamp generation logic.
  • I renamed user_attributes to prepare_otlp_attributes — this seems like a better name to me, and I found myself wanting to use user_attributes during my attempts to do more of the downstream refactors we've discussed, so I just figured it made sense to change that here.
  • I made the signature of LogfireSpan.end() compatible with trace_api.Span (required no other changes), since I think we may eventually want to rework things so LogfireSpan is returned by _ProxyTracer.start_span. Even if we don't, I think it's a fine change.

I believe this PR has no user-facing API changes.

@dmontagu dmontagu changed the title Use OPEN_SPANS for all spans Use OPEN_SPANS and custom record_exception logic for all spans Dec 18, 2024
@dmontagu dmontagu force-pushed the dmontagu/use-open-spans-for-all-spans branch from 6f48eea to cc0e3ea Compare December 18, 2024 19:56
Copy link

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

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: cf31e3b
Status: ✅  Deploy successful!
Preview URL: https://8fe87fe0.logfire-docs.pages.dev
Branch Preview URL: https://dmontagu-use-open-spans-for.logfire-docs.pages.dev

View logs

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (930fa87) to head (cf31e3b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #696   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          137       137           
  Lines        11008     11008           
  Branches      1537      1537           
=========================================
  Hits         11008     11008           

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

logfire-api/logfire_api/_internal/main.pyi Show resolved Hide resolved
logfire-api/logfire_api/_internal/tracer.pyi Show resolved Hide resolved
Comment on lines 36 to 39
class _MaybeDeterministicTimestampSpan(trace_api.Span, ReadableSpan):
class _LogfireWrappedSpan(trace_api.Span, ReadableSpan):
Copy link
Member

Choose a reason for hiding this comment

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

Rename makes sense 👍🏻

Should we make it a __slots__ data class (we should do it for all of them), using dynamic kwargs for python < 3.10, etc.? Can we another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather wait until we finish the refactoring to make it a __slots__ dataclass, just in case. But I think that makes sense once things are stable, maybe it would be fine to do it now I'd just rather not be thinking about it yet. Can change it now if you insist though.

@@ -101,13 +115,17 @@ def force_flush(self, timeout_millis: int = 30000) -> bool:


@dataclass
Copy link
Member

Choose a reason for hiding this comment

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

slots comment applies here

logfire/_internal/tracer.py Outdated Show resolved Hide resolved
logfire/_internal/tracer.py Outdated Show resolved Hide resolved
logfire/_internal/main.py Outdated Show resolved Hide resolved
logfire/_internal/config.py Outdated Show resolved Hide resolved
@alexmojaki alexmojaki enabled auto-merge (squash) December 20, 2024 09:22
@alexmojaki alexmojaki merged commit 5eced1c into main Dec 20, 2024
12 checks passed
@alexmojaki alexmojaki deleted the dmontagu/use-open-spans-for-all-spans branch December 20, 2024 09:24
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.

3 participants