Skip to content
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

ref(profiling): Deprecate hub in Profile #3270

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

szokeasaurusrex
Copy link
Member

Related to #3265

Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

I guess this is fine!

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.39%. Comparing base (2cb2dca) to head (9fe64ac).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3270      +/-   ##
==========================================
- Coverage   79.41%   79.39%   -0.03%     
==========================================
  Files         132      132              
  Lines       14263    14275      +12     
  Branches     2992     2995       +3     
==========================================
+ Hits        11327    11333       +6     
- Misses       2091     2094       +3     
- Partials      845      848       +3     
Files Coverage Δ
sentry_sdk/profiler/transaction_profiler.py 83.62% <100.00%> (+0.28%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

the working of the profiler is internal to the SDK so you are emitting a deprecation warning for our own code which makes no sense.
There is no way for the user to do something to not get this deprecation warning. Deprecation warnings are for changes the user can make.
What you want to do is to make the profiler implementation hub free.

@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Jul 11, 2024

@sl0thentr0py none of our code accesses the hub attribute, as far as I can tell, so we will not get deprecation warnings on our own code.

the working of the profiler is internal to the SDK

Where do we defined that this Profile is for internal use only? Neither the module, class, nor the attribute are underscored, so I assumed that this attribute was public API, and therefore it would be a breaking change to simply remove the attribute, since a user's code might access it (not sure why, but hypothetically this could be the case).

If it is truly internal only, I would be fine with simply removing the hub attribute and/or (if it is also internal only) the hub parameter from the constructor. But if it is not, then we cannot just remove these and we need a deprecation warning.

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

right ok, I don't think we wanted anyone to use the Profile but that's fine then

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/hub-profiler branch from 9fe64ac to d64eb24 Compare July 11, 2024 09:16
@szokeasaurusrex
Copy link
Member Author

right ok, I don't think we wanted anyone to use the Profile but that's fine then

@sl0thentr0py, I think generally something we could do better in the SDK would be to limit our public API surface and maybe more clearly define what we actually consider to be public API. For instance, perhaps we could decide that everything that we don't explicitly re-export via __all__ is considered an SDK implementation detail, or we can just get better about adding underscores to things that don't need to be public. Probably this is something that needs to wait for a major, but it would certainly make maintaining the SDK a lot easier if we had a smaller public API surface and therefore did not need to introduce quite as much backwards-compatibility code

@szokeasaurusrex szokeasaurusrex merged commit 06d5da1 into master Jul 11, 2024
122 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/hub-profiler branch July 11, 2024 09:30
arjennienhuis pushed a commit to arjennienhuis/sentry-python that referenced this pull request Sep 30, 2024
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.

3 participants