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

Monitoring de l'utilisation du pool Ecto #3465

Merged
merged 5 commits into from
Sep 18, 2023
Merged

Conversation

thbar
Copy link
Contributor

@thbar thbar commented Sep 16, 2023

Voir:

J'ai déployé le tout sur prochainement pour pouvoir en discuter au point dév de lundi.

J'ajouterai peut-être des tests, j'attends un peu voir si j'ai des retours ici:

@thbar thbar marked this pull request as ready for review September 18, 2023 07:08
@thbar thbar requested a review from a team as a code owner September 18, 2023 07:08
@thbar thbar enabled auto-merge September 18, 2023 07:21
Comment on lines +42 to +64
case measurements do
%{queue_time: queue_time} ->
Appsignal.add_distribution_value("ecto.queue_time", System.convert_time_unit(queue_time, :native, :millisecond))

_ ->
nil
end

case measurements do
%{idle_time: idle_time} ->
Appsignal.add_distribution_value("ecto.idle_time", System.convert_time_unit(idle_time, :native, :millisecond))

_ ->
nil
end

case measurements do
%{query_time: query_time} ->
Appsignal.add_distribution_value("ecto.query_time", System.convert_time_unit(query_time, :native, :millisecond))

_ ->
nil
end
Copy link
Member

Choose a reason for hiding this comment

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

Pourquoi pas un seul case avec plusieurs clauses ?

Copy link
Member

Choose a reason for hiding this comment

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

J'imagine parce que ça stop au premier match, je me demande si on pourrait écrire autrement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est parce qu'effectivement il faut avoir plusieurs match, certains events auront un, deux ou trois sous-parties, qui doivent être tracées quand elles sont présente.

L'implémentation est reprise d'ici appsignal/appsignal-elixir#318 (comment) et semble avoir été testée aussi, et AppSignal va implémenter ça en natif prochainement (appsignal/appsignal-elixir#539 (comment)), donc je n'ai pas cherché à trop modifier.

:telemetry.attach(
"transport-ecto",
# NOTE: the first two params are I believe mapped to `DB.Repo`
[:db, :repo, :query],
Copy link
Member

Choose a reason for hiding this comment

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

Ce serait pas :transport au lieu de :db ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est ce que j'avais mis initialement, mais ça ne fonctionnait pas, car le module s'appelle (en tout cas c'est ce que j'ai compris) DB.Repo.

@thbar thbar added this pull request to the merge queue Sep 18, 2023
Merged via the queue into master with commit 180f2d8 Sep 18, 2023
@thbar thbar deleted the ecto-telemetry-appsignal branch September 18, 2023 14:47
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.

2 participants