feat(data,ingestor): capture per-interval timeline snapshots#541
Conversation
New child table for per-interval timeline snapshots (issue #525): one row per participant per minute mark, FK-cascaded to matches, unique on (MatchId, ParticipantId, IntervalMinute). Adds the entity, configuration, DbSet, repository (add / delete-by-match / get-by-match), migration and regenerated compiled model. Stores raw values only — the lead vs lane opponent is a read-time delta.
Build snapshots at the 5/10/15/20/30-minute marks from the timeline frames: gold, CS, jungle CS, level, xp, damage to champions, plus cumulative kills and wards placed/killed up to each mark. Capture ward CreatorId (WARD_PLACED) for the vision signal. Persisted via delete-then-insert so re-ingestion is idempotent. Part of #525.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — feat(data,ingestor): capture per-interval timeline snapshots
Implémentation propre et bien délimitée. Le modèle de données, la migration, le compiled model et le repository sont cohérents entre eux. La logique d'idempotence (delete-then-insert) est correctement justifiée : ExecuteDeleteAsync s'exécute dans la transaction ambiante EF, évitant la race condition sur l'index unique. Les tests couvrent les cas clés (sélection de frame, cumul kills/wards, jeu court).
Aucun point bloquant identifié.
Suggestions (non bloquantes) :
SelectFrame— court-circuit possible (ligne 84) : les frames Riot étant en ordre croissant, on peutbreakdès quedeltaaugmente et éviter de scanner les frames restantes.CountUpTo— LINQ.Count()O(n) (ligne 110) : les timestamps sont insérés en ordre chronologique, donc la liste est implicitement triée. UnBinarySearchdonnerait O(log n). En pratique le volume est trop faible pour que ça compte, mais c'est à noter si la liste grossit.ToUpperInvariant()— allocations (ligne 33) : les types Riot arrivent déjà en majuscules ; une comparaisonOrdinalIgnoreCasedirecte éviterait une allocation de string par événement.
- SelectFrame breaks early once the distance to the target grows (frames ascending). - Match event types with OrdinalIgnoreCase instead of allocating via ToUpperInvariant. Part of #525.
There was a problem hiding this comment.
Résumé
Implémentation propre et bien structurée des snapshots par intervalle (issue #525). Le design est cohérent avec le reste du projet : entité, configuration EF, migration, modèle compilé, repository, builder, et tests unitaires sont tous en place et correctement câblés.
Aucun point bloquant. Trois suggestions non bloquantes ont été laissées en commentaires inline.
Suggestions (non bloquantes)
-
SelectFrame— invariant de tri implicite (TimelineSnapshotBuilder.cs:91) : lebreakearly-exit suppose que les frames sont strictement ascendantes. C'est garanti par l'API Riot, mais ce n'est nulle part affirmé dans le code. UnDebug.Assertou un commentaire liant l'hypothèse à la spec Riot renforcerait la lisibilité à long terme. -
Atomicité delete-then-insert (
TimelineIngestionService.cs:98) :ExecuteDeleteAsyncs'exécute immédiatement hors du change-tracker EF. SiSaveChangeséchoue sans transaction ambiante, les snapshots existants sont supprimés mais les nouveaux ne sont jamais persistés. En pratique le flagTimelineIngestedest rollbacké et la job retentera, donc l'impact est nul — mais ça mérite une note dans le commentaire ou une vérification que le contexte d'appel est bien transactionnel. -
Couverture des champs
XpetJungleMinionsKilleddans les tests (TimelineSnapshotBuilderTests.cs:68) : le helperParticipantne paramètre pas ces deux champs, donc leurs mappings dansTimelineSnapshotBuilderne sont jamais exercés avec une valeur non nulle. AjouterxpetjungleCsau helper permettrait de détecter un éventuel swap de champ.
- Document the ascending-frames precondition SelectFrame's early-exit relies on. - Document that the delete-then-insert empty window is transient and self-heals (TimelineIngested rolls back with a failed SaveChanges, so the match is retried). - Cover Xp and JungleMinionsKilled in the snapshot builder test to catch field swaps. Part of #525.
There was a problem hiding this comment.
Review
Implémentation propre et bien cadrée de la capture de snapshots par intervalle (issue #525). L'architecture est cohérente avec le reste du projet (delete-then-insert idempotent, FK cascade depuis matches, index unique sur la clé naturelle). Le TimelineSnapshotBuilder est correctement isolé, commenté et testé.
Aucun point bloquant.
Suggestions (non bloquantes)
WARD_KILL/ plantes de vision (l. 43) — Les destructions de Scryer's Bloom par un joueur émettent unWARD_KILLaveckillerIdvalide, ce qui peut gonfler légèrementWardsKilled. L'approximation est acceptée dans le PR description, mais une note sur la propriétéWardsKilledde l'entité clarifierait l'intention.CountUpToO(E) (l. 117) — Les listes de timestamps sont de fait triées (les events Riot arrivent en ordre chronologique). UnBinarySearchserait possible, mais la différence est négligeable pour les volumes actuels.- Test boundary tolérance (l. 46 des tests) — Ajouter un cas à exactement ±30 000 ms (inclus) et 30 001 ms (exclu) sécuriserait la constante
FrameMatchToleranceMscontre une future modification.
… kills - Boundary theory: a frame exactly +30s from a mark is captured, +1ms past is not. - Note on WardsKilled that vision-plant (Scryer's Bloom) destructions are counted. Part of #525.
There was a problem hiding this comment.
Review — feat(data,ingestor): capture per-interval timeline snapshots
L'implémentation est solide : entité bien délimitée, index unique comme garde-fou de ré-ingestion, logique de builder propre avec break anticipé dans SelectFrame, et couverture de tests pertinente (sélection de frame + comptage cumulatif + boundary de tolérance).
Aucun point bloquant identifié.
Suggestions (non bloquantes)
-
CountUpTo— scan linéaire (TimelineSnapshotBuilder.csL114-116) : les listes de timestamps sont implicitement triées (événements Riot chronologiques →Recordles ajoute dans l'ordre). UnBinarySearchdonnerait O(log N) ; sans impact à la taille actuelle des données. -
Invariant de transaction non documenté au call site (
TimelineIngestionService.csL100) :ExecuteDeleteAsyncparticipe à la transaction DB ambiante si elle existe. Si un appelant futur enveloppeApplyTimelineAsyncdansBeginTransactionAsync, le comportement « delete commit avant SaveChanges » ne tient plus. Vaut la peine de l'annoter au call site (// Must not be called inside an explicit db transaction). -
Pas de FK vers
match_participants(MatchParticipantTimelineSnapshotConfiguration.csL29) : aucune contrainte DB n'empêche unParticipantIdinvalide pour le match. Risque nul via le pipeline actuel ; à garder en tête si une suppression sélective de participants existait un jour.
1 participant
What
Capture per-interval timeline snapshots (part of #525), building on the timeline foundation from #538.
New table
match_participant_timeline_snapshotsOne row per participant per minute mark (5/10/15/20/30), FK-cascaded to
matches, unique on(MatchId, ParticipantId, IntervalMinute). Columns: gold, CS, jungle CS, level, xp, damage to champions, cumulative kills, wards placed/killed.teamPosition), consistent with the matchups design. Not stored.MatchParticipant.Ingestion
TimelineSnapshotBuilderselects the frame nearest each minute mark (within 30s, so short games simply skip later marks) and tallies cumulative kills + wards up to that frame.CreatorId(WARD_PLACED) — small extension to the timeline DTOs from feat(ingestor): capture participantFrames (positions, gold, CS, jungle) + event positions from match timeline #538.Design notes
match_participants.MatchParticipant.Testing
dotnet build --configuration Releaseclean (Data, Ingestor, IntegrationTests) — 0 warnings.TimelineSnapshotBuilderTests(frame selection + cumulative tallies).Remaining for #525 (follow-up)
Part of #525