Rest datasource trace protected (#3940)

Co-authored-by: David Glasser <[email protected]>
apollographql · Feb 5, 2021 · f63fc5a · f63fc5a
1 parent 4f441e6
commit f63fc5a
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -88,6 +88,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 ## v2.15.1
 
 - The default branch of the repository has been changed to `main`.  As this changed a number of references in the repository's `package.json` and `README.md` files (e.g., for badges, links, etc.), this necessitates a release to publish those changes to npm. [PR #4302](https://github.com/apollographql/apollo-server/pull/4302)
+- `apollo-datasource-rest`: changed `trace` method in `RESTDataSource` to `protected` to allow implementation of custom metrics.
 
 ## v2.15.0
 

diff --git a/packages/apollo-datasource-rest/README.md b/packages/apollo-datasource-rest/README.md
@@ -190,3 +190,9 @@ From our resolvers, we can access the data source and return the result:
     },
   },
 ```
+
+### Implementing custom metrics
+
+By overriding `trace` method, it's possible to implement custom metrics for request timing.
+
+See the original method [implementation](https://github.com/apollographql/apollo-server/tree/main/packages/apollo-datasource-rest/src/RESTDataSource.ts) or the reference.
diff --git a/packages/apollo-datasource-rest/src/RESTDataSource.ts b/packages/apollo-datasource-rest/src/RESTDataSource.ts
@@ -249,7 +249,7 @@ export abstract class RESTDataSource<TContext = any> extends DataSource {
     const cacheKey = this.cacheKeyFor(request);
 
     const performRequest = async () => {
-      return this.trace(`${options.method || 'GET'} ${url}`, async () => {
+      return this.trace(request, async () => {
         const cacheOptions = options.cacheOptions
           ? options.cacheOptions
           : this.cacheOptionsFor && this.cacheOptionsFor.bind(this);
@@ -278,17 +278,19 @@ export abstract class RESTDataSource<TContext = any> extends DataSource {
     }
   }
 
-  private async trace<TResult>(
-    label: string,
+  protected async trace<TResult>(
+    request: Request,
     fn: () => Promise<TResult>,
   ): Promise<TResult> {
+
     if (process && process.env && process.env.NODE_ENV === 'development') {
       // We're not using console.time because that isn't supported on Cloudflare
       const startTime = Date.now();
       try {
         return await fn();
       } finally {
         const duration = Date.now() - startTime;
+        const label = `${request.method || 'GET'} ${request.url}`;
         console.log(`${label} (${duration}ms)`);
       }
     } else {

diff --git a/packages/apollo-datasource-rest/src/__tests__/RESTDataSource.test.ts b/packages/apollo-datasource-rest/src/__tests__/RESTDataSource.test.ts
@@ -45,7 +45,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual('https://api.example.com/foo');
     });
 
@@ -64,7 +64,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
         'https://api.example.com/bar/foo',
       );
@@ -85,7 +85,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual('https://example.com/api/foo');
     });
 
@@ -110,7 +110,7 @@ describe('RESTDataSource', () => {
       fetch.mockJSONResponseOnce();
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
         'https://api-dev.example.com/foo',
       );
@@ -135,7 +135,7 @@ describe('RESTDataSource', () => {
       fetch.mockJSONResponseOnce();
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual('https://api.example.com/foo');
     });
 
@@ -161,7 +161,7 @@ describe('RESTDataSource', () => {
         offset: 20,
       });
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
         'https://api.example.com/posts?username=beyonc%C3%A9&filter=jalape%C3%B1o&limit=10&offset=20',
       );
@@ -187,7 +187,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
         'https://api.example.com/foo?a=1&api_key=secret',
       );
@@ -212,7 +212,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       // FIXME: request.credentials is not supported by node-fetch
       // expect(fetch.mock.calls[0][0].credentials).toEqual('include');
     });
@@ -237,7 +237,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.getFoo();
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].headers.get('Authorization')).toEqual(
         'secret',
       );
@@ -258,7 +258,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.postFoo({ foo: 'bar' });
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual('https://api.example.com/foo');
       expect(fetch.mock.calls[0][0].body).toEqual(
         JSON.stringify({ foo: 'bar' }),
@@ -283,7 +283,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.postFoo(['foo', 'bar']);
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual('https://api.example.com/foo');
       expect(fetch.mock.calls[0][0].body).toEqual(
         JSON.stringify(['foo', 'bar']),
@@ -319,7 +319,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.postFoo(model);
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual('https://api.example.com/foo');
       expect(fetch.mock.calls[0][0].body).toEqual(
         JSON.stringify({ foo: 'bar' }),
@@ -347,7 +347,7 @@ describe('RESTDataSource', () => {
 
       await dataSource.postFoo(form);
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual('https://api.example.com/foo');
       expect(fetch.mock.calls[0][0].body).not.toEqual('{}');
       expect(fetch.mock.calls[0][0].headers.get('Content-Type')).not.toEqual(
@@ -389,7 +389,7 @@ describe('RESTDataSource', () => {
 
         expect(data).toEqual({ foo: 'bar' });
 
-        expect(fetch.mock.calls.length).toEqual(1);
+        expect(fetch).toBeCalledTimes(1);
         expect(fetch.mock.calls[0][0].method).toEqual(method);
       });
     }
@@ -530,7 +530,7 @@ describe('RESTDataSource', () => {
 
       await Promise.all([dataSource.getFoo(1), dataSource.getFoo(1)]);
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
         'https://api.example.com/foo/1',
       );
@@ -638,7 +638,7 @@ describe('RESTDataSource', () => {
         dataSource.getFoo(1, 'anotherSecret'),
       ]);
 
-      expect(fetch.mock.calls.length).toEqual(1);
+      expect(fetch).toBeCalledTimes(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
         'https://api.example.com/foo/1?api_key=secret',
       );
@@ -764,4 +764,28 @@ describe('RESTDataSource', () => {
       });
     });
   });
+
+  describe('trace', () => {
+    it('is called once per request', async () => {
+      const traceMock = jest.fn()
+      const dataSource = new (class extends RESTDataSource {
+        baseURL = 'https://api.example.com';
+
+        getFoo() {
+          return this.get('foo');
+        }
+
+        trace = traceMock;
+      })();
+
+      dataSource.httpCache = httpCache;
+
+      fetch.mockJSONResponseOnce();
+
+      await dataSource.getFoo();
+
+      expect(traceMock).toBeCalledTimes(1);
+      expect(traceMock).toBeCalledWith(expect.any(Object),expect.any(Function));
+    });
+  })
 });