feat: use claude 3.7 to implement critical path #1493
Conversation
There was a problem hiding this comment.
This is a credible first pass. It's definitely not good enough to ship, but explores some interesting ideas. I don't like some of the architectural choices, as noted.
On a grander scale, I'd want to think about what happens with this when the root span hasn't arrived, and also when late spans arrive that might otherwise have been part of the critical path.
I'm marking it Request changes just so it can't accidentally be merged.
| @@ -1036,6 +1037,17 @@ func getDatasetFromRequest(req *http.Request) (string, error) { | |||
| return dataset, nil | |||
| } | |||
|
|
|||
| func extractParentID(ev *types.Event, cfg config.Config) string { | |||
There was a problem hiding this comment.
This is bad, turning a trace loop into an N^2 problem.
If we want to do this we would need to actually change our trace storage to a map instead of an array. Which is something we'll probably want to do anyway.
There was a problem hiding this comment.
Was this comment intended for a different function? The CalculateCriticalPath function loops through all the spans which would make it N^2, but I'm not positive this would unless the list of parent ID field names is enormous.
| ArrivalTime time.Time | ||
| IsRoot bool | ||
| IsOnCriticalPath bool | ||
| ParentID string |
There was a problem hiding this comment.
Spans already have a parentID, we don't have to invent a new one. I'd have to think about this a little harder, but we might not need to add parentID to this struct if we store spans in a map[spanID].
There was a problem hiding this comment.
Yeah, when I went about possibly doing this myself I also considered adding a ParentID here just so that we don't have to loop through the potential list. This was also more helpful when going through the trace and calculating missing spans too I believe.
| @@ -222,6 +333,25 @@ func (sp *Span) IsDecisionSpan() bool { | |||
| return isDecisionSpan | |||
| } | |||
|
|
|||
| // SpanID returns a unique identifier for this span | |||
| func (sp *Span) SpanID() string { | |||
There was a problem hiding this comment.
No. We'll extract the existing span ID. We don't want this.
| @@ -237,6 +367,12 @@ func (sp *Span) ExtractDecisionContext() *Event { | |||
| "meta.annotation_type": sp.AnnotationType(), | |||
| "meta.refinery.span_data_size": dataSize, | |||
| } | |||
|
|
|||
| // Include critical path information if this span is on the critical path | |||
| if sp.IsOnCriticalPath { | |||
There was a problem hiding this comment.
I'd also want to think about whether this belongs in a bool as part of the span struct or would be kept in a slice of spanIDs.
|
@kentquirk out of interest, I pasted your review comments (with file name + lineno) and iterated with Claude Code to try to get it to do things. It unfortunately couldn't hack it with your comments directly. Critically, it missed changing the data structure of a trace from an array of spans to a map of spans, which I imagine most of the implementation would follow from. I'm going to try and nudge it in that direction to see what happens. |
|
So far I've had to file 4 bug reports via a new session to try a corrective action based on review here. I think the most damning thing here is:
Had to correct a few other things too but it's finally churning in a direction that's not obviously wrong to me. |
THIS WAS ENTIRELY AI GENERATED
DO NOT JUST MERGE THIS
THIS WAS A TEST TO SEE IF CLAUDE 3.7 IS GOOD