feat(logger): add console, cloudWatch providers #12735

ashwinkumar6 · 2023-12-21T00:15:01Z

Description of changes

Requirements

Loggers should be configurable on Amplify singleton
Add consoleLogProvider by default, cloudWatchLogProvider is opt-in
CloudWatchLogger SDK should be tree shakeable and shouldn't be included in bundle if not opt-in

sample DX

Create cloudWatch service backend and add IAM permissions to auth, unauth roles.
Reference: check 'Add Amplify Logger resources' section

usage

import { createLogger } from "aws-amplify/logging";
import {
  getProvider as getConsoleProvider,
} from "aws-amplify/logging/console";
import {
  getProvider as getCloudWatchProvider,
} from "aws-amplify/logging/cloudwatch";

import { Amplify } from "aws-amplify";
import amplifyconfig from './amplifyconfiguration.json';

const cloudWatchProvider =  getCloudWatchProvider.initialize({
  logGroupName: "central-logger-poc-12-17",
  region: "us-west-2",
})

Amplify.configure(amplifyconfig,
  {
    Logger: {
      console: consoleProvider,
      additionalProviders: [cloudWatchProvider],
    },
  }
);

const logger = createLogger({namespace:"namespace", category:"Auth"});
newLogger.error("error bar");
newLogger.warn("warn bar");
newLogger.info("info bar");
newLogger.debug("debug bar");
newLogger.verbose("verbose bar");

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

HuiSF · 2023-12-21T01:36:20Z

packages/core/src/Logger/types.ts


-	// configure the plugin
-	configure(config?: object): object;
+export type LogParams = {


Let's use interface if there is no specific requirements of using type alias.

thanks updated all type to interface except union types and some complex types

jimblanc

Left some nits and suggestions. The bundle size increase it a little concerning for what it is, could you do an analysis on the bundle and make sure nothing is getting included that shouldn't be?

jimblanc · 2023-12-21T16:06:24Z

packages/aws-amplify/src/initSingleton.ts

@@ -23,6 +24,18 @@ export const DefaultAmplify = {
 	) {
 		let resolvedResourceConfig: ResourcesConfig;

+		// add console logger provider by default
+		if (!Amplify.libraryOptions?.Logger) {


I think we want to move this logic into the if (libraryOptions) block below & refactor as necessary

This logic is slightly updated to align with API review. We attached console logger by default here at the top.

Amplify.configure() is called for different scenarios :

no Auth config is provided

Auth options are provided

add default auth providers

if (libraryOptions) when Amplify.configure() is called multiple times

The reason we add it at the top is because we want logger category to be configured for all these scenarios.

Apart from the logic at the top, if (libraryOptions) is updated as well to handle multiple Amplify.configure(). We make sure "console" provider is always configured. Customer can update console provider config (eg disable) but not remove it

Slight updates on this to make sure we don't update the incoming libraryOptions params. Instead we create resolvedLibraryOptions and update that

jimblanc · 2023-12-21T16:10:34Z

packages/aws-amplify/src/initSingleton.ts

+				...libraryOptions,
+				Logger: {
+					...libraryOptions?.Logger,
+					providers: [consoleProvider, ...customProviders],


What happens here if a customer also provides a consoleProvider? We might need to specifically check for that before injecting the default

answered in this comment

jimblanc · 2023-12-21T16:12:09Z

packages/core/src/Logger/AdministrateLogger.ts

Can we make the Logger folder lower case to align with the others (realize it's an existing folder)?

You can do that on OSX using something like:

git mv Logger tmpLogger git mv tmpLogger logger

Also this file should be lower-case as well.

Yep this is an existing folder, makes sense to make it lower case.
updated on PR #12762

renamed this file lower case on this PR

jimblanc · 2023-12-21T16:15:51Z

packages/core/src/Logger/AdministrateLogger.ts

+import { Amplify } from '../singleton/Amplify';
+import { LogParams } from './types';
+
+export const administrateLogger = (params: LogParams) => {


Nit-pick, but don't really care for this name. Maybe something like distributeLog or sendLogToProviders 🤔 Makes it more clear what it does

makes sense, renamed to dispatchLogsToProviders

packages/core/src/Logger/ConsoleLogger.ts

packages/core/src/Logger/providers/cloudWatch/cloudWatch.ts

jimblanc · 2023-12-21T17:32:40Z

packages/core/src/Logger/providers/cloudWatch/types.ts

+export interface CloudWatchProvider extends LoggerProvider {
+	initialize: (config: CloudWatchConfig) => void;
+}


Does this DX align with the API review? Isn't it supposed to be cloudWatchProvder(config)?

Yep the DX would be as per the design, i.e

import { getConsoleProvider, } from 'aws-amplify/logger/console'; const consoleProvider = getConsoleProvider({ defaultLogLevel: 'INFO', enable:true })

Added initialize() methods to both CloudWatchProvider and ConsoleProvider as an implementation detail. tagged this as @internal and @depricated as well

jimblanc · 2023-12-21T17:44:55Z

packages/core/src/Logger/providers/console/console.ts

+	/**
+	 * @internal
+	 */


Ditto about adding some comments around what this is used for, and maybe tag both of these as @deprecated out of the gate

Thx makes sense, Added @internal and @depricated for LOG_LEVEL getter and setter.

Added @internal to all other methods in console and cloudwatch providers

jimblanc · 2023-12-21T17:53:32Z

packages/core/src/libraryUtils.ts

@@ -118,3 +118,6 @@ export {
 	SESSION_START_EVENT,
 	SESSION_STOP_EVENT,
 } from './utils/sessionListener';
+
+// logger
+export { generateLogger } from './Logger';


This should be in index.ts as it's a customer facing util (with re-export from aws-amplify)

Removed this change.
Initially we were planning on exposing logger as a util, the plan is to expose as a category instead.

Add new logger category and it's respective apis covered in #12759

packages/core/src/types/core.ts

Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>

…to console-logger-poc

jimblanc · 2024-01-03T16:24:21Z

packages/aws-amplify/src/initSingleton.ts

+				// console logger is always present
+				Logger: {
+					...libraryOptions?.Logger,
+					console:
+						libraryOptions?.Logger?.console ??
+						Amplify.libraryOptions.Logger?.console,
+				},


Is this block actually needed? The previous block on line 40 is checking "if there's no previously configured logger(s), then set up defaults & include any customer log providers", so wouldn't ...libraryOptions include it? I might be wrong, just trying to make sure I understand this logic since this function is so critical.

The previous block on line 40 is checking "if there's no previously configured logger(s), then set up defaults & include any customer log providers"

Yep that's correct, so when we do something like this for the first time both console and cloudwatch provider get added.

Amplify.configure(amplifyconfig, { Logger: { additionalProviders: [cloudWatchProvider], }, } );

But when we call configure again (same code above) console provider will get removed since libraryOptions is shallow merged. This isn't on the customer since they didn't configure console provider the first time.
wdyt

Got it, that makes sense

…to console-logger-poc

jimblanc

Looking good, few comments & nits. Also looks like bundle size still requires some tweaking. Nothing super concerning this time as long as we know where it's coming from.

jimblanc · 2024-01-10T16:11:33Z

packages/core/__tests__/Hub.test.ts

+	const module = jest.requireActual('../src/logging/ConsoleLogger');
+	return {
+		ConsoleLogger: module.ConsoleLogger,


Do we even need to mock this if we're just using the full implementation?

jimblanc · 2024-01-10T16:33:08Z

packages/core/src/logging/logger.ts

+
+	/**
+	 * Sends log event to configured logging providers
+	 *


Should tag this as @internal & @private

jimblanc · 2024-01-10T16:33:37Z

packages/core/src/logging/logger.ts

+}
+
+/**
+ * Generates a new Logger which can be used to record log events for the specified.


Specified what?

jimblanc · 2024-01-10T16:39:39Z

packages/core/src/logging/providers/cloudWatch/cloudWatch.ts

+		const client = new CloudWatchLogsClient({
+			region,
+			credentials: session.credentials,
+		});


Okay for now (since we have another task for implementing CW logger), but is this creating a new client for each log statement? I wonder if we should consider caching this & setting up a credential provider fn to keep credentials fresh.

jimblanc · 2024-01-10T16:49:32Z

packages/core/src/logging/providers/console/console.ts

+const getConsoleLogFcn = (logLevel: LogLevel) => {
+	let fcn = console.log.bind(console);
+	switch (logLevel) {
+		case 'DEBUG': {


Should VERBOSE map to DEBUG as well? Not sure how we treat it in the existing logger

yesss, Great catch @jimblanc

VERBOSE logLevel should be mapped to console.debug()

jimblanc · 2024-01-10T16:54:03Z

packages/core/src/logging/utils/loglevel.ts

+
+const logLevelIndex = ['VERBOSE', 'DEBUG', 'INFO', 'WARN', 'ERROR', 'NONE'];
+
+export const DEFAULT_LOG_LEVEL: LogLevel = 'WARN';


Should we default to WARN? 🤔 I think we use INFO in some of the function prototypes. We should also reference this value in those prototypes so it's defined in a single place.

cshfang · 2024-01-10T17:57:40Z

packages/aws-amplify/src/initSingleton.ts

@@ -6,6 +6,7 @@ import {
 	LibraryOptions,
 	ResourcesConfig,
 	defaultStorage,
+	consoleProvider,


Are we sure we don't need to contextualize this provider further given that it is exported from core root? E.g. consoleLoggingProvider?

cshfang · 2024-01-10T18:02:25Z

packages/core/__tests__/Hub.test.ts

@@ -1,3 +1,10 @@
+jest.mock('../src/logging', () => {
+	const module = jest.requireActual('../src/logging/ConsoleLogger');


Should we be doing this? Why replace with the actual module in a Hub test instead of properly mocking?

cshfang · 2024-01-10T18:43:27Z

packages/aws-amplify/__tests__/initSingleton.test.ts

 } from '@aws-amplify/core';
+import { ConsoleProvider as ConsoleProviderType } from '@aws-amplify/core/src/logging/providers/console';


Nit: Shouldn't this have been ok without the aliasing?

cshfang · 2024-01-10T18:47:44Z

packages/aws-amplify/__tests__/initSingleton.test.ts

@@ -133,7 +142,7 @@ describe('initSingleton (DefaultAmplify)', () => {

 			expect(mockAmplifySingletonConfigure).toHaveBeenCalledWith(
 				resourceConfig,
-				libraryOptions
+				{ ...libraryLoggingOptions, ...libraryOptions }


Should this spread order be reversed here and elsewhere below? I think it is reversed in the implementation if I'm not mistaken?

cshfang · 2024-01-10T18:53:42Z

packages/aws-amplify/src/initSingleton.ts

+				...resolvedLibraryOptions,
+				Logging: {
+					...resolvedLibraryOptions?.Logging,
+					// preserve console logger


QQ: How would console logger have gotten lost here?

cshfang · 2024-01-10T23:47:11Z

packages/core/src/logging/providers/console/console.ts

+	},
+};
+
+const getConsoleLogFcn = (logLevel: LogLevel) => {


Nit: Imo we should avoid the abbreviation at least in the function naming - I honestly wasn't sure what FCN stood for

cshfang · 2024-01-10T23:48:06Z

packages/core/src/logging/logger.ts

+export const createLogger = (input: CreateLoggerInput): CreateLoggerOutput => {
+	const { namespace, category } = input;
+	return new Logger(namespace, category);
+};


Nit

Suggested change

export const createLogger = (input: CreateLoggerInput): CreateLoggerOutput => {

const { namespace, category } = input;

return new Logger(namespace, category);

};

export const createLogger = ({ namespace, category }: CreateLoggerInput): CreateLoggerOutput => new Logger(namespace, category);

cshfang · 2024-01-10T23:59:38Z

packages/core/src/logging/providers/cloudWatch/cloudWatch.ts

+		const currentLevel =
+			cloudWatchConfig.loggingConstraints?.defaultLogLevel ?? DEFAULT_LOG_LEVEL;


Why is the DEFAULT_LOG_LEVEL needed? Is it because it's possible to call log before initialize? Should it be possible to do this?

cshfang · 2024-01-11T00:02:56Z

packages/core/src/logging/providers/cloudWatch/cloudWatch.ts

+		try {
+			session = await fetchAuthSession();
+		} catch (error) {
+			return Promise.reject('No credentials');


Nit: Probably should refactor as assertion functions?

cshfang · 2024-01-11T00:08:35Z

packages/core/src/logging/providers/console/console.ts

+	 * @internal
+	 * @deprecated
+	 */
+	set LOG_LEVEL(level: LogLevel) {


My understanding is that the ConsoleLogger still needs to statically have the LOG_LEVEL but why is it necessary for these providers to too?

feat(logger): add console, cloudWatch providers

7fd3480

ashwinkumar6 changed the title ~~feat(logger): add console, cloudWatch providers~~ feat(logger): add console, cloudWatch providers (draft) Dec 21, 2023

HuiSF reviewed Dec 21, 2023

View reviewed changes

ashwinkumar6 mentioned this pull request Dec 21, 2023

feat(logger): poc add support for pluggable loggers #12672

Closed

jimblanc reviewed Dec 21, 2023

View reviewed changes

ashwinkumar6 and others added 8 commits December 27, 2023 19:01

Update packages/core/src/Logger/ConsoleLogger.ts

5caf43e

Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>

Update packages/core/src/Logger/ConsoleLogger.ts

a569b67

Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>

Update packages/core/src/Logger/providers/cloudWatch/cloudWatch.ts

55b5cec

Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>

Update packages/core/src/types/core.ts

e287859

Co-authored-by: Jim Blanchard <jim.l.blanchard@gmail.com>

address feedback

2deb0ea

address feedback

e93c69d

fix: resolve conflicts

f769641

address feedback

0967c1b

ashwinkumar6 changed the title ~~feat(logger): add console, cloudWatch providers (draft)~~ feat(logger): add console, cloudWatch providers Jan 2, 2024

ashwinkumar6 requested review from HuiSF and jimblanc January 2, 2024 20:45

Ashwin Kumar added 4 commits January 2, 2024 16:08

Merge branch 'central-logger' of github.com:aws-amplify/amplify-js in…

263684b

…to console-logger-poc

chore(logger): code cleanup

61838dd

fix: TSC compliance test

3d0eef9

fix(jest): add moduleNameMapper for uuid

5da2a97

jimblanc reviewed Jan 3, 2024

View reviewed changes

Ashwin Kumar added 4 commits January 3, 2024 13:18

fix: bundle size bloat

dbd1b2e

fix(aws-amplify): unit tests

a33255d

fix: datastore unit test

f60d2da

Merge branch 'central-logger' of github.com:aws-amplify/amplify-js in…

64d1135

…to console-logger-poc

ashwinkumar6 force-pushed the console-logger-poc branch from 54be2b6 to 64d1135 Compare January 6, 2024 02:31

Ashwin Kumar added 4 commits January 6, 2024 14:20

fix: core, datastore unit tests

052d838

chore: rename dir logger to logging

0c2f4ba

feat: add additional logging tests

58bf141

chore: code cleanup

8347afa

ashwinkumar6 marked this pull request as ready for review January 8, 2024 18:04

ashwinkumar6 requested review from a team as code owners January 8, 2024 18:04

chore: fix code coverage

d764bd0

ashwinkumar6 requested a review from jimblanc January 8, 2024 23:16

Ashwin Kumar added 2 commits January 8, 2024 16:02

fix: rename library options Logger to Logging

2fcc699

chore: code cleanup

60773b0

jimblanc reviewed Jan 10, 2024

View reviewed changes

cshfang reviewed Jan 11, 2024

View reviewed changes

ashwinkumar6 closed this Mar 20, 2024


		const logLevelIndex = ['VERBOSE', 'DEBUG', 'INFO', 'WARN', 'ERROR', 'NONE'];

		export const DEFAULT_LOG_LEVEL: LogLevel = 'WARN';

		@@ -1,3 +1,10 @@
		jest.mock('../src/logging', () => {
		const module = jest.requireActual('../src/logging/ConsoleLogger');

		} from '@aws-amplify/core';
		import { ConsoleProvider as ConsoleProviderType } from '@aws-amplify/core/src/logging/providers/console';

		const currentLevel =
		cloudWatchConfig.loggingConstraints?.defaultLogLevel ?? DEFAULT_LOG_LEVEL;

feat(logger): add console, cloudWatch providers #12735

feat(logger): add console, cloudWatch providers #12735

Conversation

ashwinkumar6 commented Dec 21, 2023 • edited Loading

Description of changes

Requirements

sample DX

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblanc 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

Choose a reason for hiding this comment

ashwinkumar6 Dec 27, 2023 • edited Loading

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

ashwinkumar6 Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwinkumar6 Jan 2, 2024 • edited Loading

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

ashwinkumar6 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblanc 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

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

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

ashwinkumar6 commented Dec 21, 2023 •

edited

Loading

ashwinkumar6 Dec 27, 2023 •

edited

Loading

ashwinkumar6 Jan 2, 2024 •

edited

Loading

ashwinkumar6 Jan 2, 2024 •

edited

Loading

ashwinkumar6 Jan 4, 2024 •

edited

Loading