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

Add prometheus-client and sentry-sdk as extras #501

Merged
merged 9 commits into from
Feb 22, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Feb 17, 2022

I haven't used a specific version, because these were installed on
matrix.org and vector.im using pip install --upgrade.

David Robertson added 2 commits February 17, 2022 15:45
I haven't used a specific version, because these were installed on
matrix.org and vector.im using `pip install --upgrade`.
@DMRobertson DMRobertson marked this pull request as ready for review February 17, 2022 16:03
@DMRobertson DMRobertson requested a review from a team as a code owner February 17, 2022 16:03
@DMRobertson DMRobertson linked an issue Feb 17, 2022 that may be closed by this pull request
At least, enough annotations for the functions we use.
We only use prometheus_client for one function, but it's not annotated.
reivilibre
reivilibre previously approved these changes Feb 21, 2022
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@DMRobertson
Copy link
Contributor Author

typechecker pain not fixed GRR

@DMRobertson DMRobertson force-pushed the dmr/monitoring-extras branch from 8b1e728 to 3735867 Compare February 21, 2022 16:54
not sure why this didn't crop up before.
@DMRobertson DMRobertson force-pushed the dmr/monitoring-extras branch from 3735867 to 7fff8da Compare February 21, 2022 17:13
version = "1.5.5"
description = "Python client for Sentry (https://sentry.io)"
category = "main"
optional = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one alarmed me. The intent is to make sentry-sdk optional at runtime but required at dev-time.

But I think this is actually fine as it is. A fresh --no-dev installation created as follows didn't include sentry.

poetry shell
# nuke poetry's venv
pip freeze | cut -d " " -f 1 | xargs pip uninstall -y
poetry install --no-dev
pip list | grep 'sentry' # no results

A fresh install with dev dependencies did include sentry:

pip freeze | cut -d " " -f 1 | xargs pip uninstall -y
poetry install
...
  • Installing sentry-sdk (1.5.5)
...

And for completeness I checked what happened when installing from a wheel. That didn't install sentry.

```shell
poetry build
cd /tmp
python -m venv env
source env/bin/activate
pip install ~/workspace/sydent/dist/matrix_sydent-2.5.1-py3-none-any.whl
pip list | grep sentry  # no output

Copy link
Contributor

Choose a reason for hiding this comment

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

The intent is to make sentry-sdk optional at runtime but required at dev-time.

What's the rationale for requiring it at dev-time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type checking.

@DMRobertson DMRobertson dismissed reivilibre’s stale review February 22, 2022 10:32

New: typechecker fixups

pyproject.toml Outdated
Comment on lines 127 to 128
# sentry_sdk is required for typechecking.
sentry_sdk = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Above you write sentry-sdk with a hyphenminus; here an underscore. Should this be a hyphenminus too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you. (The distribution name is sentry-sdk; the package it provides is sentry_sdk.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's some normalisation going on behind the scenes here, but let's be consistent.

@DMRobertson DMRobertson merged commit e547352 into main Feb 22, 2022
@DMRobertson DMRobertson deleted the dmr/monitoring-extras branch February 22, 2022 14:55
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.

prometheus_client and sentry_sdk are not listed as dependencies
2 participants