CloudWatch Logs: Queries in an expression should run synchronously #64443

iwysiu · 2023-03-08T17:10:56Z

Cloudwatch Logs queries are generally run asynchronously in parts that are managed by the frontend (the frontend sends separate requests to start the query and then to poll for results). However, when a expression is used, it expects the query results to be returned from the first request. This pr adds a flag to the expression query that signals to the cloudwatch datasource that it should run the entire query synchronously in the backend and return the results (which we already handle for alerting).

The fixed bug can be reproed in the external dev as below:

Which issue(s) does this PR fix?:

Fixes #63735

Special notes for your reviewer:
Note that this doesn't solve the separate "wide format" multiple time series issue #63388, that will still require changing how we create data frames.
Also I suspect that this may be an issue in async redshift/athena.

…nchronously

grafanabot · 2023-03-08T17:12:21Z

Hello @iwysiu!
Backport pull requests need to be either:

Pull requests which address bugs,
Urgent fixes which need product approval, in order to get merged,
Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

github-actions · 2023-03-08T17:17:00Z

Backend code coverage report for PR #64443

Plugin	Main	PR	Difference
cloudwatch	82.4%	82.4%	0%

github-actions · 2023-03-08T17:17:01Z

Frontend code coverage report for PR #64443
No changes

sarahzinger

Cool catch and makes sense to me! Might be good to get someone from plugins platform to weigh-in maybe? It feels like this an interesting edge case that could be at least better documented as part of our api or something? Otherwise I'm not sure how any other plugin dev would know to do this sort of thing you know?

Also lets add tests!

sarahzinger · 2023-03-09T19:18:52Z

pkg/tsdb/cloudwatch/log_alert.go

@@ -18,7 +18,7 @@ const (
 	alertPollPeriod  = time.Second
 )

-func (e *cloudWatchExecutor) executeLogAlertQuery(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {


Now that this is used for things beyond alerting should it go in a separate file, seems a bit weird to have it in a file called log_alert probably right?

Also sort of basic on my part but how would one know if they should use a Sync vs Async Query?

renamed! We have the async queries so that the panels can get and display partial results, which the cloudwatch logs frontend handles and just keeps pulling results until it gets a done message, but the problem is that alerting and expressions want to consume the data for other things and expect the first dataQuery call to return a frame with all the data. We want to return a sync query whenever the first call needs to get the whole result.

sarahzinger · 2023-03-09T19:30:53Z

pkg/expr/nodes.go

@@ -227,6 +229,7 @@ func (dn *DSNode) Execute(ctx context.Context, now time.Time, _ mathexp.Vars, s
 		},
 		Headers: dn.request.Headers,
 	}
+	req.Headers[FromExpressionHeaderName] = "true"


how do other datasources deal with this problem? It feels worth asking grafana plugins platform folks

Asked! But honestly I think that Cloudwatch logs, redshift and athena are the only async datasources.

kylebrandt · 2023-03-10T16:56:57Z

pkg/expr/nodes.go

@@ -21,6 +21,8 @@ var (
 	logger = log.New("expr")
 )

+const FromExpressionHeaderName = "FromExpression"


Since this is a header, we generally do X-Grafana , perhaps X-Grafana-From-SSE

Oh, I'm not sure these become HTTP headers, and I guess it matches FromAlert, so okay to leave it.

iwysiu

Cool catch and makes sense to me! Might be good to get someone from plugins platform to weigh-in maybe? It feels like this an interesting edge case that could be at least better documented as part of our api or something? Otherwise I'm not sure how any other plugin dev would know to do this sort of thing you know?

It would probably make sense to document this somewhere. I actually don't think that other people are doing async datasources, at least right now, but other people might. Also, tests added! I needed to make one of the functions mock-able to do it.

iwysiu · 2023-03-10T16:24:28Z

pkg/tsdb/cloudwatch/log_alert.go

@@ -18,7 +18,7 @@ const (
 	alertPollPeriod  = time.Second
 )

-func (e *cloudWatchExecutor) executeLogAlertQuery(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {


renamed! We have the async queries so that the panels can get and display partial results, which the cloudwatch logs frontend handles and just keeps pulling results until it gets a done message, but the problem is that alerting and expressions want to consume the data for other things and expect the first dataQuery call to return a frame with all the data. We want to return a sync query whenever the first call needs to get the whole result.

iwysiu · 2023-03-10T16:28:34Z

pkg/expr/nodes.go

@@ -227,6 +229,7 @@ func (dn *DSNode) Execute(ctx context.Context, now time.Time, _ mathexp.Vars, s
 		},
 		Headers: dn.request.Headers,
 	}
+	req.Headers[FromExpressionHeaderName] = "true"


Asked! But honestly I think that Cloudwatch logs, redshift and athena are the only async datasources.

sarahzinger

🚀

grafanabot · 2023-03-13T15:36:57Z

This pull request was removed from the 9.4.5 milestone because 9.4.5 is currently being released.

…64443) (cherry picked from commit 74436d3)

…nously (#64707) CloudWatch Logs: Queries in an expression should run synchronously (#64443) (cherry picked from commit 74436d3) Co-authored-by: Isabella Siu <Isabella.siu@grafana.com>

…ously (#64443)" This reverts commit 74436d3.

…ously (#64443)" (#65036) Revert "CloudWatch Logs: Queries in an expression should run synchronously (#64443)" This reverts commit 74436d3.

…ously (#64443)" (#65036) Revert "CloudWatch Logs: Queries in an expression should run synchronously (#64443)" This reverts commit 74436d3. (cherry picked from commit 972e611)

… synchronously (#64443)" (#65067) CloudWatch Logs: Revert "Queries in an expression should run synchronously (#64443)" (#65036) Revert "CloudWatch Logs: Queries in an expression should run synchronously (#64443)" This reverts commit 74436d3. (cherry picked from commit 972e611) Co-authored-by: Shirley <4163034+fridgepoet@users.noreply.github.com>

CloudWatch Logs: queries that are part of an expression should run sy…

52e0b1e

…nchronously

grafanabot added area/backend datasource/CloudWatch labels Mar 8, 2023

iwysiu added backport v9.4.x Mark PR for automatic backport to v9.4.x add to changelog type/bug labels Mar 8, 2023

iwysiu added this to the 9.4.5 milestone Mar 8, 2023

grafanabot added the missing-labels label Mar 8, 2023

iwysiu marked this pull request as ready for review March 9, 2023 19:09

iwysiu requested review from a team as code owners March 9, 2023 19:09

iwysiu requested review from idastambuk and kevinwcyu and removed request for a team March 9, 2023 19:09

iwysiu changed the title ~~CloudWatch Logs: queries in an expression should run synchronously~~ CloudWatch Logs: Queries in an expression should run synchronously Mar 9, 2023

sarahzinger reviewed Mar 9, 2023

View reviewed changes

kylebrandt reviewed Mar 10, 2023

View reviewed changes

kylebrandt approved these changes Mar 10, 2023

View reviewed changes

add test

459a465

iwysiu commented Mar 10, 2023

View reviewed changes

iwysiu requested a review from sarahzinger March 10, 2023 19:30

iwysiu mentioned this pull request Mar 10, 2023

Update Async datasource to handle expressions grafana/grafana-aws-sdk#75

Closed

kevinwcyu approved these changes Mar 10, 2023

View reviewed changes

sarahzinger approved these changes Mar 13, 2023

View reviewed changes

grafanabot removed this from the 9.4.5 milestone Mar 13, 2023

iwysiu added this to the 9.4.6 milestone Mar 13, 2023

iwysiu merged commit 74436d3 into main Mar 13, 2023

iwysiu deleted the iwysiu/63735 branch March 13, 2023 17:31

grafanabot removed the missing-labels label Mar 13, 2023

grafanabot mentioned this pull request Mar 13, 2023

[v9.4.x] CloudWatch Logs: Queries in an expression should run synchronously #64707

Merged

grafanabot pushed a commit that referenced this pull request Mar 13, 2023

CloudWatch Logs: Queries in an expression should run synchronously (#…

c9d7756

…64443) (cherry picked from commit 74436d3)

fridgepoet added a commit that referenced this pull request Mar 20, 2023

Revert "CloudWatch Logs: Queries in an expression should run synchron…

9dbac5f

…ously (#64443)" This reverts commit 74436d3.

zerok mentioned this pull request Jun 27, 2023

zerok/9.4.x fix cl 9.4.9 #70743

Closed

gotjosh mentioned this pull request Dec 1, 2023

[v9.4.x] Alerting: Only warm alert state cache if execute_alerts=true. #78941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CloudWatch Logs: Queries in an expression should run synchronously #64443

CloudWatch Logs: Queries in an expression should run synchronously #64443

iwysiu commented Mar 8, 2023 •

edited

grafanabot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

sarahzinger left a comment

sarahzinger Mar 9, 2023

iwysiu Mar 10, 2023

sarahzinger Mar 9, 2023

iwysiu Mar 10, 2023

kylebrandt Mar 10, 2023

kylebrandt Mar 10, 2023

iwysiu left a comment

iwysiu Mar 10, 2023

iwysiu Mar 10, 2023

sarahzinger left a comment

grafanabot commented Mar 13, 2023

CloudWatch Logs: Queries in an expression should run synchronously #64443

CloudWatch Logs: Queries in an expression should run synchronously #64443

Conversation

iwysiu commented Mar 8, 2023 • edited

grafanabot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

sarahzinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

grafanabot commented Mar 13, 2023

iwysiu commented Mar 8, 2023 •

edited