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 logfire-api #268

Merged
merged 27 commits into from
Jul 5, 2024
Merged

Add logfire-api #268

merged 27 commits into from
Jul 5, 2024

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jun 18, 2024

This PR creates the logfire-api package.

We'll have to remove rye - or it needs to work with editable mode, otherwise developers need to remember to remove the .venv and run rye sync --no-lock every time they perform changes.

The approach here is simple - the logfire-api/logfire_api/__init__.py contains all the runtime logic, and the type hints are generated from the make generate-stubs command - we have a lot of pyi files.

There are two tests:

  1. One that makes sure both logfire and logfire_api have the same objects in the __init__.py.
  2. Another that makes sure we have the __init__.pyi type compliant.

@Kludex Kludex marked this pull request as draft June 18, 2024 09:39
Copy link

cloudflare-pages bot commented Jun 18, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8f42fe8
Status: ✅  Deploy successful!
Preview URL: https://c57b71f1.logfire-docs.pages.dev
Branch Preview URL: https://logfire-api.logfire-docs.pages.dev

View logs

logfire-api/src/logfire_api/__init__.py Outdated Show resolved Hide resolved
logfire-api/src/logfire_api/__init__.py Outdated Show resolved Hide resolved
logfire-api/pyproject.toml Outdated Show resolved Hide resolved
logfire-api/pyproject.toml Outdated Show resolved Hide resolved
logfire-api/README.md Outdated Show resolved Hide resolved
class Logfire:
def __init__(self, *args: Any, **kwargs: Any) -> None:
if logfire_installed:
return logfire.Logfire(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

You need to add methods like span() which return something functionally consistent with logfire.span().

And also.

def __getattr__(self, attr):
    return MagicMock()

We perhaps we shoud also implement all the know methods with a signature of (*args, **kwargs) -> None, then leave this as a fallback.

Copy link
Member

Choose a reason for hiding this comment

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

Returning a MagicMock outside of tests seems bad IMO. Can we follow the opentelemetry-api way of doing this?

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@Kludex Kludex force-pushed the logfire-api branch 2 times, most recently from 30c6e95 to d42ad35 Compare June 21, 2024 15:41
@Kludex Kludex marked this pull request as ready for review June 21, 2024 15:50
@Kludex
Copy link
Member Author

Kludex commented Jun 21, 2024

@alexmojaki do you know why those tests are failing? 🤔 There are different tests failing depending on the Python version...

Comment on lines +147 to +160
- name: Publish logfire to PyPI
uses: pypa/gh-action-pypi-publish@release/v1

- name: Build logfire-api
run: rye build
working-directory: logfire-api/

- name: Publish logfire-api to PyPI
uses: pypa/gh-action-pypi-publish@release/v1
with:
packages-dir: logfire-api/
Copy link
Member Author

Choose a reason for hiding this comment

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

I've created the publisher... Not sure if this works.

Screenshot 2024-06-21 at 17 55 49

@alexmojaki
Copy link
Contributor

I don't know why. How new/consistent are they?

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.

This looks awesome.

Thank you!

@adriangb
Copy link
Member

adriangb commented Jun 28, 2024

It seems like the idea is to generate stubs / API files from all of logfire. IMO this is unnecessary. The use case for a logfire-api is that libraries can instrument with logfire without depending on requests, etc. I think all that's needed is logfire.span(...), logfire.info(...) and maybe the things those return. It can just be something like this:

try:
    import logfire
except ImportError:
   logfire = None

def info(...) -> None:
    if logfire:
       logfire.info(...)
 
def span(...) -> ?:
    if logfire:
        return logfire.span(...)
    return ?

I leave ? as "not sure what we should return for LogfireSpan. Maybe we implement a LogfireSpanApi and have LogfireSpan inherit from it? I also think we should audit what methods/attributes are on LogfireSpan. E.g. if it gives you access to the underlaying opentelemetry.sdk.Span it maybe should be switched to opentelemetry.Span.

@Kludex
Copy link
Member Author

Kludex commented Jun 28, 2024

It seems like the idea is to generate stubs / API files from all of logfire. IMO this is unnecessary.

Since it's automated, I don't see the issue.

I think all that's needed is logfire.span(...), logfire.info(...) and maybe the things those return.

This was already discussed. Also, it was the first question before I started implementing it.

It can just be something like this:

If it were few objects, yes... But for every object on logfire/__init__.py, the current logic is easy to maintain because you don't need to add the conditionals on every logfire_api object.

@adriangb
Copy link
Member

My point is that libraries instrumenting with logfire don't need e.g. logfire.instrument_fastapi().

@alexmojaki
Copy link
Contributor

My point is that libraries instrumenting with logfire don't need e.g. logfire.instrument_fastapi().

See https://github.com/search?q=repo%3AMirascope%2Fmirascope%20instrument_openai&type=code

@samuelcolvin
Copy link
Member

Let's go ahead with this, I don't really see the point of putting effort into limiting the type hint coverage.

Let's go ahead with this.

@adriangb we have to make some decisions on calls without the whole team present, if you don't join calls you have to accept some decisions are made without you.

pyproject.toml Outdated Show resolved Hide resolved
@Kludex
Copy link
Member Author

Kludex commented Jul 5, 2024

@alexmojaki Anything else, or can I merge? I've applied all your comments.

Copy link
Contributor

@alexmojaki alexmojaki left a comment

Choose a reason for hiding this comment

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

Thanks!

@Kludex Kludex merged commit 822d554 into main Jul 5, 2024
13 checks passed
@Kludex Kludex deleted the logfire-api branch July 5, 2024 11:38
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.

None yet

4 participants