-
Notifications
You must be signed in to change notification settings - Fork 32
Enable live tailing #633
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?
Enable live tailing #633
Conversation
|
Hello @idastambuk, This is working for me locally. New logs are found via a polling mechanism. However, there is a difference in the UI: my tailed logs don’t display any colors or details, and I can't see any difference in the responses between a regular call and a WebSocket message. To summarize, I would really appreciate a review from the team to help improve this. Many thanks! |
idastambuk
left a comment
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.
Hi @nojaf, here are some initial comments, will take a look at the rest and the data frame bug separately
Thanks for submitting this!
| // The QueryDataResponse contains a map of RefID to the response for each query, and each response | ||
| // contains Frames ([]*Frame). | ||
| func (ds *OpenSearchDatasource) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { | ||
| ds.logger.Info("QueryData called", "numQueries", len(req.Queries)) |
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.
Could we do a review of all the lines where we log stuff, both FE and BE? They're good for debugging, but I think it's better to be conservative since they might be a burden on our cloud logs
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.
I'm happy to remove or update the levels afterward. The main reason for adding them was to gauge how far along we are in the pipeline.
| queryExecutor := newQueryRequest(osClient, []backend.DataQuery{backendQuery}, req.PluginContext.DataSourceInstanceSettings) | ||
| queryDataResponse, err := queryExecutor.execute(ctx) | ||
|
|
||
| if err != nil { |
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.
Query response returned contains an .Error field instead of returning an error, so this error wouldn't get logged or passed down to the user
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.
Would that be just passing things down as JSON?
json, err := respForRefId.MarshalJSON()
if err != nil {
o.logger.Error("RunStream: failed to marshal query response to JSON", "refId", refId, "error", err)
return err
}
err = sender.SendJSON(json)
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 looks good now. I think if we add the queryData() call, it will mostly take care of the error being added to the response already too
I'm not sure how to fix this, I tried it in Loki and I see the same issue not being able to expand results:
|
|
Hi @idastambuk, I think I made some progress.
The UI is still not the same as with non-live queries, though 😔 |
|
|
||
| o.logger.Info("RunStream: Polling OpenSearch", "refId", refId, "from", backendQuery.TimeRange.From, "to", backendQuery.TimeRange.To, "duration", backendQuery.TimeRange.To.Sub(backendQuery.TimeRange.From)) | ||
|
|
||
| osClient, err := client.NewClient(ctx, req.PluginContext.DataSourceInstanceSettings, o.httpClient, &backendQuery.TimeRange) |
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.
I think we we should be able to just run
queryDataResponse, err := o.QueryData(ctx, &backend.QueryDataRequest{
Queries: []backend.DataQuery{backendQuery},
PluginContext: req.PluginContext,
Headers: req.Headers,
})
instead of this and the following lines, since we have all the data to construct the data query request.
|
|
||
| lastTo := time.Now().Add(-1 * time.Second) // Start slightly in the past to catch any timing issues | ||
|
|
||
| for { |
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.
Could we extract OpenSearch polling logic (request and time range building & response processing) to a separate method on o *OpenSearchDatasource? It could return a channel, which we would just need to subscribe to in RunStream handler. It would probably be easier to add a test for opensearch streaming function it in that case then (which would be great to have!).
| return `${query.datasource?.uid}/${hashArray.map((b) => b.toString(16).padStart(2, '0')).join('')}/${config.bootData.user.orgId}`; | ||
| } | ||
|
|
||
| runLiveQueryThroughBackend(request: DataQueryRequest<OpenSearchQuery>): Observable<DataQueryResponse> { |
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.
We could also probably extract this function to a separate file to keep the datasource class cleaner
So I think this is just the feature and not a bug, it renders a basic table: LiveLogs.tsx#L146 Edit: It's a pretty old component so it is possible that it was built before the Logs visualization got the improvements. However, we're wondering if there's really a use case to have the Logs visualization while streaming, maybe the simple table is enough data. However, if you have a strong use case to display all labels along with the message during streaming, please open a feature request in the grafana repo and the Logs squad will evaluate. |
| }, fmt.Errorf("failed to get datasource info: %w", err) | ||
| } | ||
|
|
||
| o.logger.Info("SubscribeStream: path validated, authorizing stream", "path", req.Path) |
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.
We should add valiadation for OrgId (like here) too, to ensure that the requests are coming from the correct instance:





What this PR does / why we need it:
I'm trying to add live tailing / streaming.
Which issue(s) this PR fixes:
Fixes #299
Special notes for your reviewer: