Skip to content

fix: Add support for custom span filtering#859

Open
CagriYonca wants to merge 1 commit intomainfrom
fix/custom-span-filtering
Open

fix: Add support for custom span filtering#859
CagriYonca wants to merge 1 commit intomainfrom
fix/custom-span-filtering

Conversation

@CagriYonca
Copy link
Copy Markdown
Contributor

No description provided.

@CagriYonca CagriYonca self-assigned this Apr 1, 2026
@CagriYonca CagriYonca requested a review from a team as a code owner April 1, 2026 14:10
@CagriYonca CagriYonca force-pushed the fix/custom-span-filtering branch 4 times, most recently from cc66ec4 to 27614a7 Compare April 1, 2026 15:02
Copy link
Copy Markdown
Member

@pvital pvital left a comment

Choose a reason for hiding this comment

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

Minor request.

return True


def resolve_nested_key(data: Dict[str, Any], key_parts: List[str]) -> Optional[Any]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Starting with Python 3.9 (our current lower bound supported runtime version), the usage of the built-in collection types like list, dict, set, and tuple can be used directly as generic types in annotations, eliminating the need to import capitalized aliases from the typing module (line 4).

I suggest you take the opportunity and update this code to use the built-in collections.

Suggested change
def resolve_nested_key(data: Dict[str, Any], key_parts: List[str]) -> Optional[Any]:
def resolve_nested_key(data: dict[str, Any], key_parts: list[str]) -> Optional[Any]:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! (only for the parts I touched)

@CagriYonca CagriYonca force-pushed the fix/custom-span-filtering branch from 27614a7 to 2ee3887 Compare April 13, 2026 09:15
Signed-off-by: Cagri Yonca <cagri@ibm.com>
@CagriYonca CagriYonca force-pushed the fix/custom-span-filtering branch from 2ee3887 to 1924c28 Compare April 13, 2026 09:22
@CagriYonca CagriYonca requested a review from pvital April 13, 2026 09:23
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@pvital pvital left a comment

Choose a reason for hiding this comment

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

On top of the following requests, I saw your PR introduces two new maintainability issues reported by SonaQube. Please, fix them.

return True


def resolve_nested_key(data: dict[str, Any], key_parts: list[str]) -> Optional[Any]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Optional[Any] is redundant — Any already includes None. The return type is simply Any. The import of Optional can be dropped entirely.

Suggested change
def resolve_nested_key(data: dict[str, Any], key_parts: list[str]) -> Optional[Any]:
def resolve_nested_key(data: dict[str, Any], key_parts: list[str]) -> Any:

remaining = key_parts[i:]
if not remaining:
return data[candidate]
result = resolve_nested_key(data[candidate], remaining)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest that you refactor this function to use a while loop instead of a recursive function - a while loop is usually faster with constant memory usage than recursion in Python because Python is optimized for iteration, not recursion.

Every recursive call in Python pushes a new frame onto the call stack — local variables, argument bindings, return address, etc. For a deeply nested structure (say, 50 key parts), that's 50 frames allocated and deallocated. The iterative version reuses a single frame throughout. This is relatively costly in Python.

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