SARC 318 - logs et traces: jobs #111
Conversation
notoraptor
changed the title
notoraptor
force-pushed
the
sarc-318-log-trace-jobs
branch
from
abe2ed0
to
8018d7d
Compare
sarc/cli/acquire/jobs.py
Outdated
| span.add_event( | ||
| f"Acquire data on {cluster_name} for date: {date} (is_auto={is_auto})" | ||
| ) | ||
| logging.info( | ||
| f"Acquire data on {cluster_name} for date: {date} (is_auto={is_auto})" | ||
| ) |
There was a problem hiding this comment.
Ce serait mieux d'éviter les duplications vu qu'on va linker les logs avec les traces avec le logger custom pour OpenTelemetry.
There was a problem hiding this comment.
Ok ! Du coup, que garder ? Le log, ou le span event ?
sarc/jobs/sacct.py
Outdated
| @@ -115,6 +116,7 @@ def __iter__(self) -> Iterator[SlurmJob]: | |||
| print("====================================", file=sys.stderr) | |||
| print(pformat(entry), file=sys.stderr) | |||
| print("====================================", file=sys.stderr) | |||
| logger.error(f"Problem with this entry: {pformat(entry)}") | |||
There was a problem hiding this comment.
On pourrait remplacer le try-except par un with using_trace.
There was a problem hiding this comment.
Si je retire le try-except, alors les prints en cas d'except seront supprimés aussi. Or, ils sont attendus dans un test:
SARC/tests/functional/jobs/test_func_sacct.py
Line 171 in 8018d7d
Est-ce que je supprime le test aussi ?
There was a problem hiding this comment.
Hum, c'est vrai. 🤔. Si on passe entry aux attributs du span, on n'aurait pas besoin du log d'erreur. On pourrait aussi enlever les prints.
sarc/jobs/sacct.py
Outdated
| for entry in tqdm(scraper): | ||
| saved = False | ||
| if not no_prometheus: | ||
| update_allocated_gpu_type(cluster, entry) |
There was a problem hiding this comment.
Ici aussi ça serait bien d'avoir une subtrace
|
Il faudrait en ajouter aussi dans |
sarc/jobs/series.py
Outdated
| @@ -438,6 +443,7 @@ def _select_stat(name, dist): | |||
| ] | |||
|
|
|||
|
|
|||
| @trace_decorator() | |||
There was a problem hiding this comment.
Je crois que c'est correct sans ici. C'est juste un petit calcul avec pandas.
|
Il y a beaucoup de monitoring de trace qui ont été ajouté mais peu de tests pour ceux-ci en contrepartie. Idéalement, on devrait pouvoir les tester chacun sans que ça demande d´énormes tests. Peut-être si les appels à l'intérieur de la fonction pouvait être mock facilement, on pourrait facilement tester les trace séparément. On gagnerait peut-être aussi à rendre ces fonction de mock accessible globalement si on s'en sert souvent. |
|
@bouthilx pour les tests des tracers, la plupart sont testés dans Les tracers non testés sont surtout ceux ajoutés dans Ce sera un peu répétitif, mais je ne vois pas de manière plus simple de tout tester, pour le moment. En plus, si on doit faire des mocks, je ne sais pas si on peut remplacer la fonction décorée sans que ça impacte le décorateur (vu que ce dernier va retourner une fonction qui a le même nom que la fonction à décorer). Je ne sais pas trop si ça se gère facilement. Est-ce que j'y vais test par test ? Je n'aurai qu'à modifier 1 seul test pour chaque fonction de |
…taframe() and load_job_series(). All other trace decorators are tested in test_tracer_with_multiple_clusters_and_dates_and_prometheus
|
J'ai ajouté les tests manquants pour les trace decorators. Tous étaient déjà testés dans
Donc j'ai ajouté des vérifications pour ces deux traces-là. Commit: 02ea3d6 |
Break acquisition on error for a cluster, and continue to next cluster.
@bouthilx @nurbal Voici une PR pour SARC-318 (logs et traces: jobs) !