GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fixing deprecated version attribute by vagi8 · Pull Request #2338 · getsentry/sentry-python

Conversation

@vagi8

@vagi8 vagi8 commented Sep 1, 2023

Copy link
Copy Markdown
Contributor

Creating a fix for #2316

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vagi8
Fixing deprecated version attribute
6048eca
@sentrivana

sentrivana commented Sep 1, 2023

Copy link
Copy Markdown
Collaborator

Heya @vagi8, thanks for the contribution! This will work, but not in Python versions lower than 3.8 since importlib.metadata was only added in 3.8 -- and we still want to support those.

The good news is, there is already a place in the SDK where we gather all installed packages and their versions. Have a look at this:

sentry-python/sentry_sdk/integrations/modules.py

Lines 53 to 58 in 1f00437

def _get_installed_modules():
# type: () -> Dict[str, str]
global _installed_modules
if _installed_modules is None:
_installed_modules = dict(_generate_installed_modules())
return _installed_modules

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sentrivana
sentrivana requested changes Sep 1, 2023
View reviewed changes

@sentrivana sentrivana left a comment

Copy link
Copy Markdown
Collaborator

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.

See my previous comment. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vagi8

vagi8 commented Sep 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Hey @sentrivana
Thanks for bringing this to my attention.
Possible solution I can think of

  1. use importlib.metadata and pkg_resources along with a try except block inside the flask.py to get the version. (pkg_resources.get_distribution("flask").version). Similar to the '_generate_installed_modules' function but specifically for flask.
  2. Reuse the _get_installed_modules as is and extract flask version. also rename the function without the leading underscore
  3. Re-design the '_get_installed_modules' to also fetch the version for a specific module if given an input.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sentrivana

sentrivana commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

Hey @vagi8. I'd go with option 2. The _get_installed_modules function is set up so that the actual module version collection only runs once the first time it's called and is then cached in a global variable. Any subsequent calls to it will just read the global variable. So calling it from the Flask integration wouldn't mean that it would be gathering info for all modules again, if that's your concern. It's fine to call it as is.

Let's please keep the leading underscore though, the intention there is to mark it as not part of the public API.

vagi8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vagi8
getting flask version from _get_installed_modules
0891fe2
@vagi8 vagi8 closed this Sep 4, 2023
@vagi8 vagi8 reopened this Sep 4, 2023
@sentrivana
Merge branch 'master' into deprecate-version-attr
748bffd
sentrivana
sentrivana reviewed Sep 4, 2023
View reviewed changes
Comment thread sentry_sdk/integrations/flask.py Outdated
raise DidNotEnable("blinker is not installed")

installed_packages = _get_installed_modules()
FLASK_VERSION = installed_packages("flask")

@sentrivana sentrivana Sep 4, 2023

Copy link
Copy Markdown
Collaborator

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.

_get_installed_modules() returns a dict, so you'd want:

Suggested change
FLASK_VERSION = installed_packages("flask")
FLASK_VERSION = installed_packages["flask"]

I'm also thinking we could move these two lines into the setup_once method instead, so that all the version checking code is in one place.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vagi8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
vagi8 added 2 commits September 5, 2023 05:06
@vagi8
moving Flask version to setup_once
9fa946e
@vagi8
Merge branch 'deprecate-version-attr' of https://github.com/vagi8/sen…
1ac9b95
…try-python into deprecate-version-attr
sentrivana
sentrivana reviewed Sep 5, 2023
View reviewed changes
Comment thread sentry_sdk/integrations/flask.py Outdated
except ImportError:
raise DidNotEnable("blinker is not installed")

FLASK_VERSION = None

@sentrivana sentrivana Sep 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

Is this global variable needed? We only seem to be using FLASK_VERSION inside the setup_once function, so we just define and use it there?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
vagi8 and others added 3 commits September 5, 2023 07:33
@vagi8
localizing FLASK_VERSION
c900ee1
@sentrivana
Merge branch 'master' into deprecate-version-attr
39fc8f8
@sentrivana
Merge branch 'master' into deprecate-version-attr
376a8ed
sentrivana
sentrivana approved these changes Sep 6, 2023
View reviewed changes

@sentrivana sentrivana left a comment

Copy link
Copy Markdown
Collaborator

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.

Looking great @vagi8, thank you for the contribution! 🚀

One last thing: flake8 is complaining about the variable name now: sentry_sdk/integrations/flask.py:69:10: N806 variable 'FLASK_VERSION' in function should be lowercase, once we fix that we're good to merge 👍🏻

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vagi8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@vagi8
satisfying flake8
e098813
@sentrivana
sentrivana enabled auto-merge (squash) September 6, 2023 08:03
sentrivana
sentrivana reviewed Sep 6, 2023
View reviewed changes
Comment thread sentry_sdk/integrations/flask.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

auto-merge was automatically disabled September 6, 2023 08:28

Head branch was pushed to by a user without write access

@vagi8 @sentrivana
Update sentry_sdk/integrations/flask.py
b3e6520
Co-authored-by: Ivana Kellyerova <[email protected]>
@sentrivana
sentrivana enabled auto-merge (squash) September 6, 2023 08:55
@sentrivana
Merge branch 'master' into deprecate-version-attr
62855d9
@sentrivana
sentrivana merged commit 0fb0dea into getsentry:master Sep 6, 2023
@sentrivana sentrivana mentioned this pull request Sep 6, 2023
use importlib.metadata.version to get package version #2316
Closed
sentrivana added a commit that referenced this pull request Sep 18, 2023
@vagi8 @sentrivana
Fixing deprecated version attribute (#2338)
597c59e
---------

Co-authored-by: Ivana Kellyerova <[email protected]>
This was referenced Nov 4, 2026
package_version() parses every installed package's metadata, slowing sentry_sdk.init() #8190
Closed
perf: Avoid scanning installed packages in package_version #8191
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sentrivana sentrivana sentrivana approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@vagi8 @sentrivana
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.