Repository navigation
gh-160976: Preserve source file path in Gecko sampling collector - #160982
makwana-akshat wants to merge 3 commits into
Conversation
|
cc @ivonastojanovic ? |
ivonastojanovic
left a comment
There was a problem hiding this comment.
Thanks for the fix! I’ve left a couple of nits.
One other thing I was wondering about is the resource name. _get_or_create_resource still uses the basename, so both files would show up as Resource: __init__.py in the profiler. Since Gecko resources are meant to group functions, for example by library or origin, would it make more sense to use the module name for the resource instead?
cc @pablogsal
| self.assertGreater(stack_table["length"], 0) | ||
| self.assertGreater(len(stack_table["frame"]), 0) | ||
|
|
||
| def test_gecko_collector_preserves_source_file_paths(self): |
There was a problem hiding this comment.
I think we could simplify this test quite a bit. Some of the assertions seem redundant, like checking the index types/ranges and that both the indices and strings are different. I think we could keep just the assertions that verify the behavior we actually care about here.
| @@ -0,0 +1,2 @@ | |||
| Fixed an issue in :mod:`profiling.sampling` where the Gecko exporter (``--gecko``) | |||
| only recorded source file basenames instead of preserving the source file path. | |||
There was a problem hiding this comment.
nit: Maybe a bit shorter, e.g. “The Gecko reporter (--gecko) in profiling.sampling now records full source file paths instead of only basenames.”
|
Thanks for the review! I've simplified the regression test to focus on resolving each function's filename to the correct full source path and ensuring the two files are distinguished. I've also shortened the NEWS entry. Regarding |
Fixes #160976.
Summary
The Gecko sampling profiler exporter used only the basename of Python source filenames, losing directory information when different files shared the same basename.
This PR: