Prefix requestId with data source id #27
Conversation
iwysiu
requested review from
sarahzinger and
njvrzm
and removed request for
a team
| @@ -85,7 +93,7 @@ export class DatasourceWithAsyncBackend< | |||
| let allData: DataFrame[] = []; | |||
|
|
|||
| return getRequestLooper( | |||
| { ...request, targets: [target], requestId: `aws_ts_${this.requestCounter++}` }, | |||
| { ...request, targets: [target], requestId: `${this.requestIdPrefix}_${this.requestCounter++}` }, | |||
There was a problem hiding this comment.
Instead of using the instance id as the prefix, timestream uses a global requestCounter to avoid conflicting requestIDs from different timestream datassources, I don't really have a strong opinion on how we distinguish them
There was a problem hiding this comment.
one potential issue with using a global here is different versions of this module being loaded and the issue could still be present.
a (very) edge case scenario: athena and redshift both use DatasourceWithAsyncBackend with a global counter, but they end up being bundled with different versions of the @grafana/async-data-query package. a dashboard with an athena and redshift panel would load different DatasourceWithAsyncBackend modules and they both generate the same requestId.
There was a problem hiding this comment.
Instead of using the instance id as the prefix, timestream uses a global requestCounter to avoid conflicting requestIDs from different timestream datassources, I don't really have a strong opinion on how we distinguish them
So does timestream not use this package but athena/redshift do? Maybe timestream should also preface requests with the datasource UID? Seems like anything we can do to make this more streamline could be good. Why did we reference aws_ts_ before here?
Also lets add a test!
There was a problem hiding this comment.
So does timestream not use this package but athena/redshift do? Maybe timestream should also preface requests with the datasource UID? Seems like anything we can do to make this more streamline could be good. Why did we reference aws_ts_ before here?
We were referencing aws_ts_ before because the code for looping the requests was taken from timestream, and the requestID wasn't updated, we don't use this package in timestream. It may make sense to update it, but it doesn't have the issues that Kevin referenced for why the async one isn't a global, so timestream is probably fine.
| @@ -85,7 +93,7 @@ export class DatasourceWithAsyncBackend< | |||
| let allData: DataFrame[] = []; | |||
|
|
|||
| return getRequestLooper( | |||
| { ...request, targets: [target], requestId: `aws_ts_${this.requestCounter++}` }, | |||
| { ...request, targets: [target], requestId: `${this.requestIdPrefix}_${this.requestCounter++}` }, | |||
| @@ -106,4 +113,17 @@ describe('DatasourceWithAsyncBackend', () => { | |||
| expect(queryMock).toHaveBeenCalledTimes(1); | |||
| expect(queryMock).toHaveBeenCalledWith(defaultRequest); | |||
| }); | |||
|
|
|||
| it('uses the datasource id for the request id', () => { | |||
There was a problem hiding this comment.
This test checks that we're sending the request id with the datasource id
| @@ -90,4 +91,38 @@ describe('requestLooper', () => { | |||
| scheduler.flush(); | |||
| expect(onCancel).toBeCalledTimes(1); | |||
| }); | |||
|
|
|||
| it('increments the request id', (done) => { | |||
There was a problem hiding this comment.
This test confirms that we use the passed in requestId and appends the count for successive async queries
| const getRequestLooperMock = jest.fn(); | ||
| jest.mock('./requestLooper.ts', () => ({ | ||
| ...jest.requireActual('./requestLooper.ts'), | ||
| getRequestLooper: (req: any, options: any) => getRequestLooperMock(req, options), |
Because different async datasources were creating queries with the same prefix, they were creating overlapping requestIds where every query but the last one was getting canceled by
backendSrv. Also because the requestID code had been taken from timestream, they were overlapping with timestream query request ids. This changes the code to use the datasource id as the prefix, which should be unique.Part of grafana/timestream-datasource#273