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 fastapi arguments attributes directly on the root otel span #509

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

alexmojaki
Copy link
Contributor

@alexmojaki alexmojaki commented Oct 17, 2024

Having a separate "FastAPI arguments" span is a pain. It costs extra money and requires writing a join in SQL to combine info about the root span (particularly the duration) with the argument values. I want to get rid of it.

As a first step, this PR takes the same attributes that are set on the fastapi args span and sets them on the root span created by OTEL. This allows people to use the root span in queries and avoid joining in SQL, and should ease the transition to removing the fastapi args span.

In a followup change I will remove the fastapi args span. We might want to treat this as a breaking change and release version 2.0. We might also want to add a parameter (off by default) that keeps the span, although the only reasons I can think of to keep it are quite weak:

  1. Backwards compatibility in dashboard/alert queries
  2. Grouping together spans that come from resolving arguments (e.g. db queries)
  3. Seeing how long it takes to resolve arguments

Copy link

cloudflare-workers-and-pages bot commented Oct 17, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: cecc093
Status: ✅  Deploy successful!
Preview URL: https://72333100.logfire-docs.pages.dev
Branch Preview URL: https://alex-fastapi-args-root-span.logfire-docs.pages.dev

View logs

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6d7bb92) to head (cecc093).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #509   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          132       132           
  Lines        10056     10082   +26     
  Branches      1360      1364    +4     
=========================================
+ Hits         10056     10082   +26     

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

@alexmojaki alexmojaki requested review from Kludex and dmontagu and removed request for dmontagu October 17, 2024 13:40
@alexmojaki alexmojaki marked this pull request as draft October 18, 2024 15:31
@Kludex
Copy link
Member

Kludex commented Oct 21, 2024

Nice!

@@ -58,7 +58,6 @@ def instrument_fastapi(
dict[str, Any] | None,
]
| None = None,
use_opentelemetry_instrumentation: bool = True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This option was a bad idea on my part, clutters the docs and API, and doesn't really have a purpose any more now that OTEL kwargs are passed through.

@alexmojaki alexmojaki marked this pull request as ready for review October 21, 2024 10:40
@alexmojaki
Copy link
Contributor Author

After chatting with @dmontagu I think it's likely that we won't remove the fastapi args span soon, but this change should be useful anyway.

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

We might also want to add a parameter (off by default) that keeps the span

I don't think this is necessary...

@JacobHayes
Copy link

JacobHayes commented Oct 22, 2024

Ah, this might also fix #533 if it also removes the http.method and http.route attributes from the FastAPI Arguments span.


Separately, I do find it somewhat useful to track the db queries that come from parsing arguments and resolving Depends (especially since optimizing those would often help multiple endpoints), but would be fine if that was dropped.

@alexmojaki
Copy link
Contributor Author

Separately, I do find it somewhat useful to track the db queries that come from parsing arguments and resolving Depends

But @JacobHayes if you've instrumented db queries then you'll still see spans for those queries even if we drop the fastapi args span, they just won't all be grouped together. Otherwise you'll see fastapi args taking a long time to resolve but you won't know why.

@alexmojaki alexmojaki enabled auto-merge (squash) October 24, 2024 15:35
@alexmojaki alexmojaki merged commit 83e1c34 into main Oct 24, 2024
18 checks passed
@alexmojaki alexmojaki deleted the alex/fastapi-args-root-span branch October 24, 2024 15:37
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