[Analytics] Suivi de l'Apdex et des requêtes non abouties (Sentry) #5162
Conversation
celine-m-s
added
the
no-changelog
celine-m-s
force-pushed
the
celinems/metrics
branch
from
63d738d
to
9da79db
Compare
celine-m-s
force-pushed
the
celinems/metrics
branch
from
9da79db
to
f63200c
Compare
celine-m-s
force-pushed
the
celinems/metrics
branch
from
f63200c
to
3a07f6c
Compare
celine-m-s
force-pushed
the
celinems/metrics
branch
from
3a07f6c
to
0d644fb
Compare
tests/analytics/conftest.py
Outdated
| @pytest.fixture | ||
| def sentry_respx_mock(respx_mock): | ||
| url = f"{settings.API_SENTRY_BASE_URL}/organizations/{settings.API_SENTRY_ORG_NAME}/events/" | ||
| respx_mock.get(url).mock(return_value=httpx.Response(200, json=_events_for_metrics_endpoint_json_response())) |
There was a problem hiding this comment.
Peut-être juste renvoyer respx_mock.get(url) et laisser les tests explicitement définir ce que renvoie l'API ?
There was a problem hiding this comment.
👍 pour retourner la Route créée par respx, ça permettra à chaque test de surcharger le retour si besoin.
Après sur le fait de mettre un retour par défaut, je dirais que ça dépend du nombre de test qui ont un intérêt à utiliser la valeur par défaut et/ou si celle-ci est assez large/correcte pour être utilisée par défaut.
There was a problem hiding this comment.
Seuls trois tests utilisent cette fixture et toujours avec ce payload-là. Le répéter dans chaque test me semble un peu en avance de phase. Il sera toujours possible de changer par la suite si le besoin s'en fait sentir.
tests/analytics/conftest.py
Outdated
| @pytest.fixture | ||
| def sentry_respx_mock(respx_mock): | ||
| url = f"{settings.API_SENTRY_BASE_URL}/organizations/{settings.API_SENTRY_ORG_NAME}/events/" | ||
| respx_mock.get(url).mock(return_value=httpx.Response(200, json=_events_for_metrics_endpoint_json_response())) |
There was a problem hiding this comment.
👍 pour retourner la Route créée par respx, ça permettra à chaque test de surcharger le retour si besoin.
Après sur le fait de mettre un retour par défaut, je dirais que ça dépend du nombre de test qui ont un intérêt à utiliser la valeur par défaut et/ou si celle-ci est assez large/correcte pour être utilisée par défaut.
tests/utils/apis/test_sentry_api.py
Outdated
| def test_request(respx_mock): | ||
| url = f"{settings.API_SENTRY_BASE_URL}/organizations/{settings.API_SENTRY_ORG_NAME}/events/" | ||
| respx_mock.get(url).mock(return_value=httpx.Response(200, json=_events_for_metrics_endpoint_json_response())) |
There was a problem hiding this comment.
C'est pas équivalent à ce que fait sentry_respx_mock ? 🤔
There was a problem hiding this comment.
Si mais je n'arrive pas à réutiliser la fixture car elle a été définie dans le module analytics et non au global. Ça m'ennuyait de polluer le conftest.py global pour trois petits tests et les quelques solutions que j'ai pu trouver sur les internets me semblent vraiment compliquées pour pas grand chose.
Mais je suis preneuse de toute bonne idée car moi aussi ça me gêne ! 💡
| assert settings.API_SENTRY_STATS_TOKEN in response.request.headers["authorization"] | ||
| assert urllib.parse.quote(start.isoformat()) in str(response.url) | ||
| assert urllib.parse.quote(end.isoformat()) in str(response.url) |
There was a problem hiding this comment.
PI, respx permet de ne créer une route qui ne répond que quand certain prédicat sont vrai, par exemple un paramètre GET ou un header, ça permet ensuite de choper les oublis via assert_all_called ou assert_all_mocked ;). #CeintureBretelles
There was a problem hiding this comment.
J'ai fait une proposition dans le dernier commit.
celine-m-s
force-pushed
the
celinems/metrics
branch
from
0d644fb
to
80c4fcd
Compare
Afin de suivre l'impact technique de nos actions, nous récoltons l'apdex et le taux de requêtes en échec de Sentry et les conservons dans l'app Analytics.
cf #5162