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

Clarification du code custom Telemetry AppSignal Ecto #4280

Merged
merged 5 commits into from
Oct 29, 2024

Conversation

thbar
Copy link
Contributor

@thbar thbar commented Oct 29, 2024

En lien avec:

Et vu que c'était peu clair pour moi, je clarifie le code sur les points suivants:

  • le handler ne sert en fait qu'à AppSignal (pour l'intégration Ecto) et non à "AppSignal + Ecto"
  • le module ne sert actuellement qu'à AppSignal, le côté dynamique du handler n'a pa été exploité à ce stade, donc je prends note de cet état de fait et je renomme le module pour que ça soit plus clair

Vu que le code correspondant n'est pas testé et pas simple à tester en pratique, j'ai testé en local à la main (dans handle_event) avec un IO.inspect(measurements, IEx.inspect_opts()), aucun souci détecté, ce qui veut dire que le handler est bien enregistré et fonctionne (point important car ceci n'est pas testé à ma connaissance actuellement).

When encountering #4279, I initially wondered if this could impact our metrics (by detaching the corresponding handler).

But it turns out this handler is solely focused on AppSignal custom Ecto integration.

Also renaming the handler itself, since it shows up in the logs.
@thbar thbar added the dette technique Entretien & maintenance générale, nécessaire pour que le code reste de bonne qualité label Oct 29, 2024
@thbar thbar requested a review from a team as a code owner October 29, 2024 08:18
@thbar
Copy link
Contributor Author

thbar commented Oct 29, 2024

Merci @ptitfred pour la review. Je déploie et j'irai jeter un oeil dans AppSignal après.

@thbar thbar added this pull request to the merge queue Oct 29, 2024
Merged via the queue into master with commit 45f0c29 Oct 29, 2024
4 checks passed
@thbar thbar deleted the appsignal-ecto-cleanup branch October 29, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dette technique Entretien & maintenance générale, nécessaire pour que le code reste de bonne qualité
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants