Fix/Improve logging API, validation, and doc (#492)

Author: bboure

doc/general-config.md

@@ -47,7 +47,7 @@ appSync:
 - `substitutions`: See [Substitutions](substitutions.md)
 - `caching`: See [Cacing](caching.md)
 - `waf`: See [Web Application Firefall](WAF.md)
-- `log`: See [Logs](#Logs)
+- `logging`: See [Logging](#Logging)
 - `defaultMappingTemplates`:
   - `request`: Optional. A default request mapping template filename for all resolvers.
   - `response`: Optional. A default response mapping template filename for all resolvers.
@@ -176,17 +176,18 @@ Old-style descriptions (using `#`) are supported by AppSync but will be removed
 
 Types can implement multiple [interfaces](https://spec.graphql.org/October2021/#sec-Interfaces) using an ampersand `&` in GraphQL, but AppSync uses the old comma (`,`) separator. `&` is the only separator suported by this plugin, but it will automatically be replaced with a `,`.
 
-## Logs
+## Logging
 
 ```yaml
 appSync:
   name: my-api
-  log:
+  logging:
     level: ERROR
-    logRetentionInDays: 14
+    retentionInDays: 14
 ```
 
 - `level`: `ERROR`, `NONE`, or `ALL`
-- `excludeVerboseContent`: Boolean. Exclude or not verbose content.
-- `logRetentionInDays`: Number of days to retain the logs.
+- `enabled`: Boolean, Optional. Defaults to `true` when `logging` is present.
+- `excludeVerboseContent`: Boolean, Optional. Exclude or not verbose content (headers, response headers, context, and evaluated mapping templates), regardless of field logging level. Defaults to `false`.
+- `retentionInDays`: Optional. Number of days to retain the logs. Defaults to [`provider.logRetentionInDays`](https://www.serverless.com/framework/docs/providers/aws/guide/serverless.yml#general-function-settings).
 - `roleArn`: Optional. The role ARN to use for AppSync to write into CloudWatch. If not specified, a new role is created by default.

src/__tests__/api.test.ts

@@ -33,10 +33,10 @@ describe('Api', () => {
     it('should compile the Api Resource with logs enabled', () => {
       const api = new Api(
         given.appSyncConfig({
-          log: {
+          logging: {
             level: 'ERROR',
             excludeVerboseContent: false,
-            logRetentionInDays: 14,
+            retentionInDays: 14,
           },
         }),
         plugin,
@@ -344,19 +344,35 @@ describe('Api', () => {
   });
 
   describe('Logs', () => {
-    it('should not compile CloudWatch Resources when disabled', () => {
+    it('should not compile CloudWatch Resources when logging not configured', () => {
       const api = new Api(given.appSyncConfig(), plugin);
       expect(api.compileCloudWatchLogGroup()).toMatchInlineSnapshot(
         `Object {}`,
       );
     });
 
+    it('should not compile CloudWatch Resources when logging is disabled', () => {
+      const api = new Api(
+        given.appSyncConfig({
+          logging: {
+            level: 'ERROR',
+            retentionInDays: 14,
+            enabled: false,
+          },
+        }),
+        plugin,
+      );
+      expect(api.compileCloudWatchLogGroup()).toMatchInlineSnapshot(
+        `Object {}`,
+      );
+    });
+
     it('should compile CloudWatch Resources when enaabled', () => {
       const api = new Api(
         given.appSyncConfig({
-          log: {
+          logging: {
             level: 'ERROR',
-            logRetentionInDays: 14,
+            retentionInDays: 14,
           },
         }),
         plugin,

src/__tests__/validation/__snapshots__/base.test.ts.snap

@@ -14,9 +14,9 @@ exports[`Valdiation Domain Invalid should validate a Invalid Route 53 1`] = `
 `;
 
 exports[`Valdiation Log Invalid should validate a Invalid 1`] = `
-"/log/level: must be \\"ALL\\", \\"ERROR\\" or \\"NONE\\"
-/log/logRetentionInDays: must be integer
-/log/excludeVerboseContent: must be boolean"
+"/logging/level: must be one of 'ALL', 'ERROR' or 'NONE'
+/logging/retentionInDays: must be integer
+/logging/excludeVerboseContent: must be boolean"
 `;
 
 exports[`Valdiation Waf Invalid should validate a Invalid 1`] = `

src/__tests__/validation/base.test.ts

@@ -18,7 +18,7 @@ describe('Valdiation', () => {
           name: 'Minimum',
           config: {
             ...basicConfig,
-            log: {
+            logging: {
               level: 'ALL',
             },
           } as AppSyncConfigInput,
@@ -27,9 +27,9 @@ describe('Valdiation', () => {
           name: 'Full',
           config: {
             ...basicConfig,
-            log: {
+            logging: {
               level: 'ALL',
-              logRetentionInDays: 14,
+              retentionInDays: 14,
               excludeVerboseContent: true,
               loggingRoleArn: { Ref: 'MyLogGorupArn' },
             },
@@ -50,9 +50,9 @@ describe('Valdiation', () => {
           name: 'Invalid',
           config: {
             ...basicConfig,
-            log: {
+            logging: {
               level: 'FOO',
-              logRetentionInDays: 'bar',
+              retentionInDays: 'bar',
               excludeVerboseContent: 'buzz',
               loggingRoleArn: 123,
             },

src/resources/Api.ts

@@ -92,15 +92,15 @@ export class Api {
       });
     }
 
-    if (this.config.log) {
+    if (this.config.logging && this.config.logging.enabled !== false) {
       const logicalIdCloudWatchLogsRole =
         this.naming.getLogGroupRoleLogicalId();
       set(endpointResource, 'Properties.LogConfig', {
-        CloudWatchLogsRoleArn: this.config.log.roleArn || {
+        CloudWatchLogsRoleArn: this.config.logging.roleArn || {
           'Fn::GetAtt': [logicalIdCloudWatchLogsRole, 'Arn'],
         },
-        FieldLogLevel: this.config.log.level,
-        ExcludeVerboseContent: this.config.log.excludeVerboseContent,
+        FieldLogLevel: this.config.logging.level,
+        ExcludeVerboseContent: this.config.logging.excludeVerboseContent,
       });
     }
 
@@ -112,7 +112,7 @@ export class Api {
   }
 
   compileCloudWatchLogGroup(): CfnResources {
-    if (!this.config.log) {
+    if (!this.config.logging || this.config.logging.enabled === false) {
       return {};
     }
 
@@ -132,7 +132,7 @@ export class Api {
             ],
           },
           RetentionInDays:
-            this.config.log.logRetentionInDays ||
+            this.config.logging.retentionInDays ||
             this.plugin.serverless.service.provider.logRetentionInDays,
         },
       },

src/types/plugin.ts

@@ -20,7 +20,7 @@ export type AppSyncConfig = {
   pipelineFunctions: Record<string, PipelineFunctionConfig>;
   substitutions?: Substitutions;
   xrayEnabled?: boolean;
-  log?: LogConfig;
+  logging?: LoggingConfig;
   caching?: CachingConfig;
   waf?: WafConfig;
   tags?: Record<string, string>;
@@ -296,10 +296,11 @@ export type VisibilityConfig = {
   sampledRequestsEnabled?: boolean;
 };
 
-export type LogConfig = {
+export type LoggingConfig = {
   level: 'ERROR' | 'NONE' | 'ALL';
+  enabled?: boolean;
   excludeVerboseContent?: boolean;
-  logRetentionInDays?: number;
+  retentionInDays?: number;
   roleArn?: string | IntrinsicFunction;
 };
 

src/validation.ts

@@ -731,17 +731,18 @@ export const appSyncSchema = {
         },
       },
     },
-    log: {
+    logging: {
       type: 'object',
       properties: {
         roleArn: { $ref: '#/definitions/stringOrIntrinsicFunction' },
         level: {
           type: 'string',
           enum: ['ALL', 'ERROR', 'NONE'],
-          errorMessage: 'must be "ALL", "ERROR" or "NONE"',
+          errorMessage: "must be one of 'ALL', 'ERROR' or 'NONE'",
         },
-        logRetentionInDays: { type: 'integer' },
+        retentionInDays: { type: 'integer' },
         excludeVerboseContent: { type: 'boolean' },
+        enabled: { type: 'boolean' },
       },
       required: ['level'],
     },