-
Notifications
You must be signed in to change notification settings - Fork 77
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 local
parameter to logfire.configure()
#508
Conversation
@@ -889,7 +901,6 @@ def get_tracer_provider(self) -> ProxyTracerProvider: | |||
Returns: | |||
The tracer provider. | |||
""" | |||
self.warn_if_not_initialized('No logs or spans will be created') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved these warnings to prevent logfire.instrument_fastapi
emitting similar warnings 3 times.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #508 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 132 132
Lines 9961 9986 +25
Branches 1341 1343 +2
=========================================
+ Hits 9961 9986 +25 ☔ View full report in Codecov by Sentry. |
AioHttpClientInstrumentor().instrument(**kwargs) # type: ignore[reportUnknownMemberType] | ||
AioHttpClientInstrumentor().instrument( # type: ignore[reportUnknownMemberType] | ||
**{ | ||
'tracer_provider': logfire_instance.config.get_tracer_provider(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn on how to do this. On the one hand I don't want to completely prevent users from passing custom providers. We even do that in our own code (passing a NoopMeterProvider to disable fastapi metrics), and I've recommended another user to do the same. On the other hand I don't want to encourage or document it. So this makes it secretly possible to override the logfire providers with custom ones. Which is a mess for type checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we're using kwargs already so this seems like a step better
@@ -1262,7 +1265,13 @@ def instrument_sqlalchemy(self, **kwargs: Unpack[SQLAlchemyInstrumentKwargs]) -> | |||
from .integrations.sqlalchemy import instrument_sqlalchemy | |||
|
|||
self._warn_if_not_initialized_for_instrumentation() | |||
return instrument_sqlalchemy(**kwargs) | |||
return instrument_sqlalchemy( | |||
**{ # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same logic as the other instrumentations, I just realized at this point that this approach was a bit easier and less tedious.
Deploying logfire-docs with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks this looks great!
Closes #195