chore: track relationship_inferrence_phase_ms#7780
Conversation
paula-stacho
added
no release notes
There was a problem hiding this comment.
Pull request overview
Adds a new telemetry metric to Data Modeling analysis events to capture just the relationship inference phase duration.
Changes:
- Extends Data Modeling telemetry event payloads with
relationship_inference_phase_ms(optional). - Measures and propagates relationship inference phase duration from
analyzeCollections()into tracked events for create/redo flows. - Updates inline event documentation to describe the new metric.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/compass-telemetry/src/telemetry-events.ts | Adds optional relationship_inference_phase_ms to multiple Data Modeling telemetry event payload types and documents the metric. |
| packages/compass-data-modeling/src/store/analysis-process.ts | Tracks relationship inference duration inside analyzeCollections() and includes it in telemetry for diagram creation / re-analysis outcomes. |
| @@ -305,6 +306,8 @@ export function startAnalysis( | |||
| options, | |||
| }) | |||
| ); | |||
| const { collections, relations } = result; | |||
| relationsInferencePhaseMs = result.relationsInferencePhaseMs; | |||
There was a problem hiding this comment.
relationship_inference_phase_ms will only be populated if analyzeCollections() resolves successfully. If the analysis fails/cancels during relationship inference (or earlier inside analyzeCollections()), relationsInferencePhaseMs remains undefined, even though the telemetry event types now allow reporting it for Failed/Cancelled events. If the intent is to capture inference duration for those outcomes too, consider capturing the inference timing in analyzeCollections() via a try/finally around the inference block and surfacing it on failure (e.g., attach to the thrown error, dispatch a dedicated action containing the duration, or move telemetry for failure/cancel to where the duration is known).
There was a problem hiding this comment.
I kinda agree with this. Do we really need relationsInferencePhaseMs in failed/cancelled events?
There was a problem hiding this comment.
ah right. yeah I'll remove it, I was following the analysis_time_ms and it didn't hit me. thanks!
There was a problem hiding this comment.
actually.. it might be nice to have it (if available), I'll look into including the number in case of failures instead
There was a problem hiding this comment.
for example if the user waited for 45 mins and then cancelled the DM creation - and the last 40 mins have been relationships.. that'd be good for us to know.
There was a problem hiding this comment.
Kind of. After its available, we only await on getInitialLayout.
There was a problem hiding this comment.
Feel free to merge as is
There was a problem hiding this comment.
Updated to ensure the failures also have the last phase time
paula-stacho
force-pushed
the
relationship_infer_time
branch
from
979cfe1 to
3853284
Compare
3 participants
Description
Another small addition to DM telemetry - relationship_inference_phase_ms. The other two phases overlap.