Skip to content

Conversation

svartalf
Copy link
Contributor

Description

Without "@functools.wraps" added, Ray exposes Prometheus metrics with all tasks named "new_func"

Issues

  • Follow up to !4430 comments

Reminders

Without "@functools.wraps" added, Ray exposes Prometheus metrics with all tasks named "new_func"
@svartalf svartalf requested a review from a team as a code owner September 24, 2025 09:25
default=None,
)
)
new_func.__signature__ = signature.replace(parameters=params)
Copy link

Choose a reason for hiding this comment

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

Bug: Tracing Parameter Patching Causes Signature Errors

The _tracing parameter patching logic unconditionally appends the parameter, which can lead to two issues: a ValueError if user_f already defines _tracing, or an invalid signature if **kwargs is present, as keyword-only parameters must precede **kwargs.

Fix in Cursor Fix in Web

Comment on lines +84 to +94
signature = inspect.signature(new_func)
params = list(signature.parameters.values())
params.append(
inspect.Parameter(
"_tracing",
kind=inspect.Parameter.KEYWORD_ONLY,
default=None,
)
)
new_func.__signature__ = signature.replace(parameters=params)

Choose a reason for hiding this comment

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

Potential bug: Adding the _tracing parameter without checking for its existence can cause a ValueError if the user's function already has a parameter with that name, crashing the application.
  • Description: The Ray integration modifies the signature of a user's function to add a _tracing parameter. However, it does not first check if a parameter with that name already exists. If a user decorates a function that already has a _tracing parameter, the code attempts to add a duplicate. This will cause inspect.signature.replace() to raise a ValueError because duplicate parameter names are not allowed. This error is unhandled and will crash the application when the function is decorated, preventing the application from starting.

  • Suggested fix: Before appending the _tracing parameter to the params list, iterate through the existing parameters to check if one with the name _tracing already exists. If it does, do not append the new parameter.
    severity: 0.7, confidence: 0.95

Did we get this right? 👍 / 👎 to inform future reviews.

@svartalf
Copy link
Contributor Author

Follow-up to the CI run:

  • mypy failure can be solved by adding # type: ignore[attr-defined]. I'm postponing adding a commit with it in case if we would want to include anything else after the discussion
  • Not sure what to do with LLM suggestions, as introducing a "hidden" parameter for tracing propagation seems to be the way to do things?
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Hey @svartalf, thanks for the PR! TL;DR, it all looks good to me, but see below for a couple of optional suggestions.

Re: LLM suggestions, we could rename the arg to _sentry_tracing or something less generic to reduce the likelihood of collisions. FWIW I don't think it's necessary, so feel free to ignore.

If you can add a test or integrate a check for this into one of the tests in the existing test suite, that'd be awesome.

Approving already though since it looks good to me as is, pending the type: ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants