Skip to content

Add CodeSource parameter to configure - #451

Merged
Kludex merged 5 commits into
mainfrom
add-code-source
Oct 7, 2024
Merged

Kludex merged 5 commits into
mainfrom
add-code-source

Conversation

@Kludex

@Kludex Kludex commented Sep 26, 2024

Copy link
Copy Markdown
Member

I'll add documentation after this is supported by the frontend.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 26, 2024

Copy link
Copy Markdown

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 11b8fb0
Status: ✅  Deploy successful!
Preview URL: https://55b7eeb7.logfire-docs.pages.dev
Branch Preview URL: https://add-code-source.logfire-docs.pages.dev

View logs

@codecov

codecov Bot commented Sep 26, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1386152) to head (11b8fb0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #451   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          131       131           
  Lines         9783      9811   +28     
  Branches      1290      1293    +3     
=========================================
+ Hits          9783      9811   +28     

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

"""The root path for the source code in the repository.

Example:
If the `code.filename` is `/path/to/project/src/logfire/main.py` and the `root_path` is `src/`, the URL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a long and not very readable example.

It's code.filepath, not filename.

code.filepath is not necessarily something users will know about. It's relative to the current working directory, so it probably shouldn't start with /. And users are likely to not know what the current working directory is or will be when running the app, so setting this correctly may be tricky.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Suggestions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm planning to document this feature btw...

This PR is just the first step. After the feature is on the frontend, I'll add to the docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Logfire spans automatically have a code.filepath attribute indicating where they were created. That path is relative to the current working directory of the Python process. The root_path setting is combined with the code.filepath attribute to construct a link, so root_path should be a path starting from the root of the repository that corresponds to the current working directory of the Python process.

For example, suppose that your repository contains a/b/c/main.py, the folder a/b/ is copied into the /docker/root/ folder of your docker container, and within the container the command python ./b/c/main.py is run from within the /docker/root/a/ directory. Then code.filepath will be b/c/main.py for spans created in that file, and the root_path should be set to a so that the final link is a/b/c/main.py.

revision: str
"""The git revision of the code e.g. branch name, commit hash, tag name etc."""

root_path: str

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this default to ''?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should I default to .?



@dataclass
class CodeSource:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be CodeSourceOptions? I genuinely don't know.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It just seems cleaner the way it is, even if the others have the Options.

@Kludex

Kludex commented Oct 2, 2024

Copy link
Copy Markdown
Member Author

@alexmojaki Your comments seem not that blocking, can you check them and tell me what would make you happy? I want to push this forward.

@Kludex

Kludex commented Oct 2, 2024

Copy link
Copy Markdown
Member Author

We can even put the feature under experimental or something?

@alexmojaki

Copy link
Copy Markdown
Contributor

I think just document the dataclass and parameter as not ready for public use and then it's fine to go ahead.

Or just don't change the SDK for now if it's only going to add resource attributes for the sake of developing the frontend, those can already be set in environment variables.

@Kludex
Kludex requested a review from alexmojaki October 7, 2024 07:25
@Kludex
Kludex merged commit 6962166 into main Oct 7, 2024
@Kludex
Kludex deleted the add-code-source branch October 7, 2024 10:15
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.

2 participants