plugin(logs): support time based queries for Splunk - #1317
Conversation
|
|
|
/ok-to-test |
42a7364 to
c7bee96
Compare
|
1. Split into two PRs This PR bundles two independent changes — it would be easier to review as two separate PRs: PR 1: Add time-range filtering to Splunk queries PR 2: Make UID keys configurable via 2. Use The new UID key settings are read via |
|
@enarha thanks for the review I'll start working on your points accordingly |
… Splunk queries This commit represents Part 1 of the requested PR split. It focuses on the straightforward performance fix for Splunk queries without mixing in the configuration changes for UID keys. Changes include: - Extracting `getLogRequestParams()` from the existing `getLokiLogs()` code (the logic that resolves uidKey, startTime, endTime from the record). - Using the extracted `getLogRequestParams()` in both `getLokiLogs()` and `getSplunkLogs()`. - Adding `earliest_time` and `latest_time` to the Splunk search job request so Splunk queries do not search all-time when the run's time window is already known.
c7bee96 to
15d052d
Compare
Extract getLogRequestParams() from getLokiLogs() to share record time and UID key resolution between Loki and Splunk, and add earliest_time/latest_time to Splunk search queries to scope searches to the run's time window.
|
/lgtm |
There was a problem hiding this comment.
Pull request overview
This PR updates the v1alpha2 log plugin’s Splunk backend to use record-derived start/end timestamps when creating Splunk search jobs, limiting query scope and reducing load on Splunk.
Changes:
- Refactors time/UID extraction into a shared
getLogRequestParamshelper for Loki and Splunk. - Adds
earliest_timeandlatest_timeparameters to Splunk job creation requests. - Extends the Splunk log plugin unit test to assert the submitted time bounds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/api/server/v1alpha2/plugin/plugin_logs.go | Adds shared log request parameter extraction and applies record-based time bounds to Splunk queries. |
| pkg/api/server/v1alpha2/plugin/plugin_logs_test.go | Adds assertions that Splunk requests include earliest/latest time values derived from the record. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data := &pipelinev1.PipelineRun{} | ||
| err := json.Unmarshal(rec.Data, data) | ||
| if err != nil { | ||
| err = fmt.Errorf("failed to marshal pipelinerun data for fetching log, err: %s", err.Error()) | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", fmt.Errorf("failed to marshal pipelinerun data for fetching log, err: %w", err) | ||
| } |
There was a problem hiding this comment.
This comment make sense ; but at the same time, the error message was already there before this PR, so we can definitely fix this in a follow-up instead.
| data := &pipelinev1.TaskRun{} | ||
| err := json.Unmarshal(rec.Data, data) | ||
| if err != nil { | ||
| err = fmt.Errorf("failed to marshal taskrun data for fetching log, err: %s", err.Error()) | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", fmt.Errorf("failed to marshal taskrun data for fetching log, err: %w", err) | ||
| } |
| if data.Status.CompletionTime == nil { | ||
| err = errors.New("there's no completion in pipelinerun") | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", errors.New("there's no completion in pipelinerun") | ||
| } | ||
| endTime = strconv.FormatInt(data.Status.CompletionTime.Add(s.forwarderDelayDuration).UTC().Unix(), 10) |
There was a problem hiding this comment.
I think it's by design though.
| if data.Status.CompletionTime == nil { | ||
| err = errors.New("there's no completion in taskrun") | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", errors.New("there's no completion in taskrun") | ||
| } | ||
| endTime = strconv.FormatInt(data.Status.CompletionTime.Add(s.forwarderDelayDuration).UTC().Unix(), 10) |
There was a problem hiding this comment.
I think it's by design though.
| @@ -263,6 +265,9 @@ func TestSplunkLogs(t *testing.T) { | |||
| // Verify the request path and query parameters | |||
| switch r.URL.Path { | |||
| case "/services/search/v2/jobs": | |||
| r.ParseForm() | |||
| gotEarliestTime = r.FormValue("earliest_time") | |||
| gotLatestTime = r.FormValue("latest_time") | |||
| data := &pipelinev1.PipelineRun{} | ||
| err := json.Unmarshal(rec.Data, data) | ||
| if err != nil { | ||
| err = fmt.Errorf("failed to marshal pipelinerun data for fetching log, err: %s", err.Error()) | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", fmt.Errorf("failed to marshal pipelinerun data for fetching log, err: %w", err) | ||
| } |
There was a problem hiding this comment.
This comment make sense ; but at the same time, the error message was already there before this PR, so we can definitely fix this in a follow-up instead.
| data := &pipelinev1.TaskRun{} | ||
| err := json.Unmarshal(rec.Data, data) | ||
| if err != nil { | ||
| err = fmt.Errorf("failed to marshal taskrun data for fetching log, err: %s", err.Error()) | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", fmt.Errorf("failed to marshal taskrun data for fetching log, err: %w", err) | ||
| } |
| if data.Status.CompletionTime == nil { | ||
| err = errors.New("there's no completion in taskrun") | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", errors.New("there's no completion in taskrun") | ||
| } | ||
| endTime = strconv.FormatInt(data.Status.CompletionTime.Add(s.forwarderDelayDuration).UTC().Unix(), 10) |
There was a problem hiding this comment.
I think it's by design though.
| if data.Status.CompletionTime == nil { | ||
| err = errors.New("there's no completion in pipelinerun") | ||
| s.logger.Error(err) | ||
| return err | ||
| return "", "", "", errors.New("there's no completion in pipelinerun") | ||
| } | ||
| endTime = strconv.FormatInt(data.Status.CompletionTime.Add(s.forwarderDelayDuration).UTC().Unix(), 10) |
There was a problem hiding this comment.
I think it's by design though.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
time based queriesand customUIDKeyEnvthe documentation changes accordinglystartTime and endTime for Splunk queriespipelineRunUIDKeyEnv, taskRunUIDKeyEnvRelease Notes
Splunk queries do now contain
startTimeandendTimeto limit the range result returned by Splunk.The default values are taken from the stored Results record.
For pull requests with a release note:
additional note for reviewers
Since I am not the best at writing go code, the code snippets are AI powered. I reviewed them and they seem to be adequate.