-
Notifications
You must be signed in to change notification settings - Fork 164
[PLAT-334] AI reports #8673
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
base: main
Are you sure you want to change the base?
[PLAT-334] AI reports #8673
Conversation
| } | ||
|
|
||
| const llmRequestTimeout = 60 * time.Second | ||
| const llmRequestTimeout = 90 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After all the tool calls, sometimes it would take more than 1 minute for LLM to synthesize the final output. So had to increase it.
| bool intervals_check_unclosed = 15; | ||
| // AI report configuration | ||
| string format = 17; // "query" (default) or "ai_session" | ||
| AIReportConfig ai_config = 18; // Configuration for AI-powered reports (only used when format = "ai_session") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at alerts, they have generic resolver and resolver_properties fields:
rill/proto/rill/admin/v1/api.proto
Lines 3340 to 3345 in fd7a010
string resolver = 13; google.protobuf.Struct resolver_properties = 14; // DEPRECATED: Use resolver and resolver_properties instead. string query_name = 3; // DEPRECATED: Use resolver and resolver_properties instead. string query_args_json = 4; rill/proto/rill/runtime/v1/resources.proto
Lines 649 to 650 in fd7a010
string resolver = 22; google.protobuf.Struct resolver_properties = 23;
We also intended to do the same for reports, but we never got around to it. However, with this change, rather than hard-code the AI resolver properties here, can we take a generic approach similar to alerts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the change for ai report, need to fix the export request and download api for standard report
| // AITimeRange defines a time range using ISO 8601 duration strings. | ||
| // The time range is resolved at report execution time. | ||
| message AITimeRange { | ||
| string iso_duration = 1; // ISO 8601 duration (e.g., "P7D" for 7 days, "P1M" for 1 month) | ||
| string iso_offset = 2; // Optional ISO 8601 offset for comparison ranges (e.g., "P7D" to offset by 7 days) | ||
| string time_zone = 3; // IANA timezone (e.g., "America/New_York") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears not to support Rill time expressions? I believe the UI only uses those for new reports (or otherwise, they will probably migrate to that soon).
Instead of a custom type, can it use our main TimeRange proto type, which we already have utils for converting into a concrete time range? Thinking about this one:
rill/proto/rill/runtime/v1/queries.proto
Lines 610 to 627 in fd7a010
| // 2 of the (start, end, iso_duration) should be set | |
| message TimeRange { | |
| // Optional. Defaults to min | |
| google.protobuf.Timestamp start = 1; | |
| // Optional. Defaults to max | |
| google.protobuf.Timestamp end = 2; | |
| // Optional, ie PT1M | |
| string iso_duration = 3; | |
| // Optional, ie PT1M | |
| string iso_offset = 4; | |
| TimeGrain round_to_grain = 5; | |
| // Optional. IANA format, ie Europe/Copenhagen. Defaults to UTC | |
| string time_zone = 6; | |
| // Optional. Rill format time range. Should only be used for alerts and reports. | |
| // For dashboard call ResolveTimeRanges. | |
| string expression = 7; | |
| string time_dimension = 8; // Optional. If not specified, falls back to the primary time dimension in the metrics view spec | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aa far as I remember, at the time of coding UI was not using expressions so used this, related to this during tool calls LLM would most of the times use expressions in wrong way like adding other fields along with it, causing validation errors. Was not aware its the way going forward, I can make that change and possibly add some concrete examples with expression so that it uses them correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would always lead to failed tool calls thus wasting tokens
runtime/ai/analyst_agent.go
Outdated
| // Add scheduled insight mode context | ||
| data["is_scheduled_insight"] = args.IsScheduledInsight | ||
| data["is_scheduled_insight_user_prompt"] = args.IsScheduledInsight && !(strings.EqualFold(strings.TrimSpace(args.Prompt), "Generate the scheduled insight report.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Doing a
strings.EqualFoldseems kind of unsafe in case we change that text upstream. Would it be possible to keep the prompt empty (so we can doif eq .prompt ""), and inject the "Generate ..." prompt inuserPromptwhen prompt is empty andIsScheduledInsightis true? - Nit: Can you move this up to the initial
data :=statement like the other props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a custom flag to indicate if user has provided custom prompt. The reason for not injecting userPrompt is because it won't be visible in session messages then, if we want in session msgs then we will have add router and agent call msgs along with their full json args.
It won't matter much anyways as per discussion with Nishant, we would always want a prompt here so I think as user keep on selecting time period, measures and dims in the report modal, UI can dynamically create prompt like it does for Explain feature - prompt can be like this for comparative analysis
You are doing comparative analysis between two time periods <t1, t2> in scheduled insight report mode, your analysis should:
1. Compare current period to the comparison period for <all key> or <m1, m2...> measures
2. Identify which measures changed significantly (>10%)
3. For each significant change, identify the dimensional drivers
4. Highlight any ranking changes in top dimensions
5. Generate 3-5 key insights with supporting charts
Focus areas:
- **Overall changes**: Which measures changed the most between periods?
- **Drivers of change**: Which dimensions contributed most to increases/decreases?
- **Ranking shifts**: Did any top dimensions change rank significantly?
- **Anomalies**: Any unusual patterns unique to one period?
For single period analysis -
You are doing analysis on time period <t1> in scheduled insight report mode, your analysis should:
1. Show totals for the <most impactful> or <m1, m2...> measures in the period
2. Identify interesting trends within the time range (use time series)
3. Find anomalies - unusual spikes, drops, or outliers
4. Highlight top performers and notable dimension values
5. Generate 3-5 key insights with supporting charts
Focus areas:
- **Totals**: What are the key numbers for this period?
- **Trends**: How did metrics change over the period? Any acceleration/deceleration?
- **Anomalies**: Are there any unusual data points that stand out?
- **Distribution**: Which dimensions dominate? Any concentration issues?
runtime/reconcilers/report.go
Outdated
| case "email": | ||
| recipients := pbutil.ToSliceString(notifier.Properties.AsMap()["recipients"]) | ||
| for _, recipient := range recipients { | ||
| opts := &email.ScheduledReport{ | ||
| ToEmail: recipient, | ||
| ToName: "", | ||
| DisplayName: rep.Spec.DisplayName, | ||
| ReportTime: t, | ||
| DownloadFormat: formatExportFormat(rep.Spec.ExportFormat), | ||
| } | ||
| urls, ok := meta.RecipientURLs[recipient] | ||
| if !ok { | ||
| return false, fmt.Errorf("failed to get recipient URLs for %q", recipient) | ||
| } | ||
| opts.OpenLink = urls.OpenURL | ||
| u, err := createExportURL(urls.ExportURL, t) | ||
| if err != nil { | ||
| return false, err | ||
| if rep.Spec.Format == "ai_session" { | ||
| reports := make(map[string]*aiReport) | ||
| for _, recipient := range recipients { | ||
| urls, ok := meta.RecipientURLs[recipient] | ||
| if !ok { | ||
| return false, fmt.Errorf("failed to get recipient URLs for %q", recipient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is getting pretty nested and hard to follow. Can you think of a way to simplify the flows?
Depending on your thoughts on my earlier comment about having a single SendScheduledReport function, maybe it can build the messages and send the messages in two separate steps, and become simpler that way?
| func buildAISessionURL(baseOpenURL, sessionID string) string { | ||
| // replace {session_id} in the baseOpenURL with the actual sessionID | ||
| return strings.ReplaceAll(baseOpenURL, "%7Bsession_id%7D", sessionID) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a little hacky, had to jump back to the URL generation to check things here. Would be nice at least with a comment about this feature in the URLs generation function.
But also, I'm thinking if there's a way we can make this a bit more generic/clean. Another thing we need to support soon is precomputed report exports. Which is kind of similar to a pre-created AI session. I wonder if thinking about that use case as well could give us a cleaner path here.
For example, in alerts, we store the alert history in AlertState. Maybe we should keep similar history for reports? And then keep the existing reports/{name}/open link and let the UI redirect? Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment at url generation place. You meant to store resolver and its corresponding meta id like session id in case of ai session in ReportExecution proto?
runtime/resolvers/ai.go
Outdated
| // resolveTimeRange resolves the time range from ISO duration to actual timestamps. | ||
| func (r *aiResolver) resolveTimeRange() (start, end time.Time, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seems to duplicate functionality from rilltime. It would be really nice if we could centralize on the rilltime implementation and not have time range resolution logic elsewhere.
See the code here for an example of how rilltime is used:
| rillTime, err := rilltime.Parse(tr.Expression, rilltime.ParseOptions{ |
On reconciliation, an AI resolver is created which generates an AI session with
analyst_agentand a slightly modified system prompt for scheduled report, upon competition it include a summary and send out the session id link to the recipient.Report modes behaviour
Recipient mode - In this mode, AI report is run with each recipient attributes and a separate session is created for them.
Creator mode - In this mode, a single session is created with owners attribute (without any charts as it required mv agg for rendering), this session is a shared session and a magic token is used for viewing this session (this mgc token has no access just used for authentication), for continuing conversation user should be logged in and conversation should be forked.
Format
Reports can now use
format: ai_sessionto generate AI-powered insights instead of traditional query exports. Example YAML:Checklist: