-
Notifications
You must be signed in to change notification settings - Fork 515
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
docs: Clarify that instrumenter
is internal-only
#3299
docs: Clarify that instrumenter
is internal-only
#3299
Conversation
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 13439 tests with View the full list of failed testspy3.12-common
py3.12-gevent
py3.13-common
|
0e2ba1d
to
330985a
Compare
sentry_sdk/client.py
Outdated
warnings.warn( | ||
"The `instrumenter` option will be removed in the next major version.", | ||
DeprecationWarning, | ||
stacklevel=3, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also emit deprecation warnings for people using OTel now, right? The problem with that is that there is nothing actionable for the user here -- they can't do anything to get rid of the warning. So they'll be pestered by this message that basically says "you're using the SDK wrong" and can't do anything about it.
I'd either get rid of the warning completely, or give it a bit more context so that folks know what to expect ("...the option will be removed since OpenTelemetry tracing will become the default" or something like that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah fair point, I am not really sure the best way to go about this. The downside of no warning is that we will drop this option without ever having had a depreciation warning for it – the downside of including the warning is that people might be confused about what action they need to take.
Maybe we should just highlight in the docs that instrumenter
will be removed in the next major? Perhaps, we can also plan to add a deprecation warning in the final 2.x minor/patch release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is what I will do: I will split the deprecation warning into a separate PR that we can keep around for when we add it in the final 2.x release. This PR will only contain the docstring updates to clarify that we will be dropping instrumenter
in 3.x. Then, I will prepare a docs PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation PR is here: #3308
330985a
to
3533784
Compare
instrumenter
init
optioninstrumenter
is internal-only
Adjust docstrings of all non-deprecated functions which take an `instrumenter` parameter to state that `instrumenter` is only meant to be used by the SDK, and that it is deprecated for client code. The docstrings also inform users that `instrumenter` will be removed in the next major release.
4ad56dc
to
825b2b9
Compare
Adjust docstrings of all non-deprecated functions which take an `instrumenter` parameter to state that `instrumenter` is only meant to be used by the SDK, and that it is deprecated for client code. The docstrings also inform users that `instrumenter` will be removed in the next major release.
Adjust docstrings of all non-deprecated functions which take an
instrumenter
parameter to state thatinstrumenter
is only meant to be used by the SDK, and that it is deprecated for client code. The docstrings also inform users thatinstrumenter
will be removed in the next major release.