Add correlation id in routerlicious-driver #4734
Conversation
| @@ -54,17 +56,17 @@ export class DeltaStorageService implements IDeltaStorageService { | |||
| to?: number): Promise<ISequencedDocumentMessage[]> { | |||
| const query = querystring.stringify({ from, to }); | |||
|
|
|||
| let headers: { Authorization: string } | null = null; | |||
| const headers: OutgoingHttpHeaders = { | |||
| "x-correlation-id": uuid.v4(), | |||
There was a problem hiding this comment.
What's the value of this ID?
I can see the value if it was written to client side telemetry, then we can use it to correlate activity between client & server. But with this code, it will land only on server, so server can generate ID with the same result, no?
There was a problem hiding this comment.
That's a good point. We added correlation id in server side and this PR is just for the completeness for the server side to receive http requests with correlation id in the header. As for client side telemetry, I didn't go deep into that. Could you explain a little bit more how we can add to client side telemetry? Or maybe we can talk offline with Tanvir.
There was a problem hiding this comment.
@vladsud Correct! FRS has recently done the work to store and track correlation ids of http requests. Next, we want wrap all these calls with telemetry. Can you provide us some pointers on how ODSP driver does that? I am expecting to follow a similar pattern.
There was a problem hiding this comment.
Take a look at how ITelemetryLogger / ITelemetryBaseLogger and usage in ODSP driver.
Basically it is passed to driver in Container.load() (when calling createDocumentService) and driver passes it through it's layers and uses to log data (look for this.logger in odspDocumentStorageManager.ts as an example).
We wrap pretty much every network call and write out a bunch of properties to allow us to do analyzes later.
Note that in similar case, we rely on server communicating to client its correlation ID (in http response), and client recording it in client-side logging. That way, server is pretty much agnostic of client and client behavior. Reverse would work too, I do not have strong opinion here.
| headers = { | ||
| Authorization: `Basic ${fromUtf8ToBase64(`${tenantId}:${storageToken.jwt}`)}`, | ||
| }; | ||
| headers.Authorization = `Basic ${fromUtf8ToBase64(`${tenantId}:${storageToken.jwt}`)}`; |
There was a problem hiding this comment.
Nit: Why do we need this change?
There was a problem hiding this comment.
The initial value of headers was null. Now the initial value of headers is { "x-correlation-id": uuid.v4() } (on line 63). So it needs to add a key to dictionary instead of giving a value to it.
There are 3 functions for routerlicious-driver to talk with server-routerlicious in documentService.ts: connectToStorage(), connectToDeltaStorage(), connectToDeltaStream(). This PR is to address adding correlation id in connectToDeltaStorage() as it goes deeper to deltaStorageService.ts. To address connectToStorage() is in another PR here as it uses historian client and sends http calls using RestWrapper. To address correlation id in connectToDeltaStream() is not in the scope now as it uses socket connections.