Logger & LoggerMiddleware #66

AllanZhengYP · 2017-11-09T01:48:19Z

Just the simple logger class that enable users to define a logger level. And a logger middleware to log the operation name and input & output parameters. This middleware won't log anything unless the logOperationInfo is set to true

jeskew · 2017-11-09T02:46:12Z

packages/logger/src/Formatter.ts

+        return res;
+    }
+
+    private parsePattern(pattern: string): string[] {


This seems like a very heavy solution. Since you know which keys have been provided to you, why not just use String.prototype.replace, e.g., formatString.replace(/{operation}/g, operation)?

This parsing function is all based on my assumption that we don't have prior knowledge of keys in the pattern string. Admittedly this is a heavy solution, it is only called in the constructor. You are right, if we have a pre-defined set of keys in the pattern string, the implementation will be much simpler.

jeskew · 2017-11-09T02:49:19Z

packages/logger/src/Formatter.ts

+        this.literalArray = this.parsePattern(pattern);
+    }
+
+    format(stats: {[key: string]: string|undefined}): string {


Allowing any key to be supplied means that individual applications will need to develop their own idioms for which keys to supply when calling the logger and which to expect when writing format strings. If we do end up using a structured formatter like this, I would recommend using an interface with known keys.

For example, Ruby defines a fixed set of substitution patterns that will be replaced by the log formatter (http://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/Log/Formatter.html).

jeskew · 2017-11-09T02:50:10Z

packages/logger/src/Formatter.ts

+
+    private parsePattern(pattern: string): string[] {
+        let isExpression: boolean = false;
+        let res: Array<string> = [];


Please use full word names for all variables other than loop counters.

jeskew · 2017-11-09T02:53:15Z

packages/logger/src/Formatter.ts

+        let tmpStr = '';
+        for(let i = 0; i < pattern.length; i++) {
+            let char = pattern.charAt(i);
+            if (isExpression && char !== '}') {


Since keys may be any string, you will need to support escaping control characters; \} should be interpreted as the string } and not as the end of an expression.

jeskew · 2017-11-09T02:54:13Z

packages/logger/src/Formatter.ts

+                isExpression = false;
+            } else if (
+                !isExpression && 
+                !(i < pattern.length - 1 && pattern.substring(i, i + 2) === '${')


Why use a two token sequence to mark the start of expressions? You could use { instead of ${, which would make control character escaping simpler. Right now, if a customer wanted to define a log pattern inside of a backtick-quoted string, they would need to escape replacement sequences meant for the log formatter instead of for the JS template string formatter (e.g., User: ${username}; Operation: \${operation}, where the customer wants to use JS substitution for variables in scope).

Good catch. When design this I just wanted it to look like string template but never thought about that users may use backticks.

jeskew · 2017-11-09T03:38:31Z

packages/remove-sensitive-logs/src/index.ts

+        return undefined
+    }
+    if (shape.sensitive) {
+        return '***SensitiveInformation***'


Can we just use ***?

jeskew · 2017-11-09T03:39:13Z

packages/remove-sensitive-logs/src/index.ts

+    SerializationModel
+} from '@aws/types';
+
+export function mapToShape(obj: any, shape: SerializationModel): any {


I don't think we should be exposing functions with a return type of any.

packages/remove-sensitive-logs/src/index.ts

jeskew · 2017-11-09T03:41:35Z

packages/types/src/logger.ts

+
+export type LogLevel = 'info' | 'log' | 'warn' | 'error';
+
+export namespace Logging {


This package should only contain interfaces and types. Exporting a namespace will create a runtime value.

jeskew · 2017-11-09T03:42:07Z

packages/types/src/logger.ts

+    logLevels?: [LogLevel] | LogLevel
+}
+
+export interface WritableLogger {


Couldn't this just be called Logger?

jeskew · 2017-11-14T02:35:13Z

packages/logger/src/index.ts

+    }
+
+    private write(content: string, logLevel: LogLevel) {
+        if (this.logLevels.indexOf(logLevel) >= 0 && !!this.logger[logLevel]) {


The cast isn't necessary here, as the placement of this.logger[logLevel] as a clause in an if condition will automatically cause it to be evaluated for truthiness. If you do want an explicit cast, please use Boolean(variable) instead of !!.

jeskew · 2017-11-14T02:35:56Z

packages/logger/src/index.ts

+
+    private getLogLevels(logLevel: LogLevel): [LogLevel] {
+        const allLogLevels: [LogLevel] = [LogLevel.Log, LogLevel.Info, LogLevel.Warn, LogLevel.Error];
+        return Array.prototype.slice.call(allLogLevels, allLogLevels.indexOf(logLevel), allLogLevels.length); 


I'm not a huge fan of this approach. Why can't we just use a numeric scale like Log4j, Apache, or Syslog?

jeskew · 2017-11-15T00:48:06Z

packages/middleware-operation-logging/src/index.ts

+    constructor(
+        private readonly next: Handler<any, any>,
+        private readonly context: HandlerExecutionContext,
+        private readonly paramsOperation: paramsOperation


This will need to be the first constructor argument if you want people to be able to override it. (End users must be able to use this constructor to get to a constructor signature that complies with the middleware interface.) With this constructor, using this class as middleware will cause a compilation error.

jeskew · 2017-11-15T00:50:41Z

packages/types/src/logger.ts

+    info(content: string): void;
+}
+
+export interface paramsOperation {


Interface names should start with a capital letter, even if the shape they define is a function. The name could also be a bit more descriptive -- there's no indication here what a paramsOperation function should do.

I use this name because they are operations used on parameters. The operations don't need to be removing sensitive info, but also can be flattening the structure and so on.

True, but "operation used on parameters" is a generic enough statement that it could be applied to any code.

What's important here is what the operation does to those parameters. I think a better name here would be "SensitiveDataCensor" or "SensitiveDataScrubber" -- something that conveys what the function is expected to do.

jeskew · 2017-11-15T00:52:16Z

packages/types/src/logger.ts

@@ -0,0 +1,27 @@
+import {Shape, Member} from './protocol'
+
+export enum LogLevel {


enums are objects at runtime and can't be used in the types package. I think what you want is a string literal union here ('log'|'info'|'warn'|'error') and then an enum inside the logger package mapping each of those strings to an integer.

jeskew · 2017-11-15T00:52:41Z

packages/types/src/logger.ts

+    Error = 'error'
+}
+
+export interface LoggerOption {


This should be plural (LoggerOptions).

jeskew · 2017-11-15T00:52:59Z

packages/types/src/logger.ts

+}
+
+export interface Logger {
+    logOperationInfo: boolean;


What is this property for?

jeskew · 2017-11-15T00:53:19Z

packages/types/src/logger.ts

@@ -0,0 +1,27 @@
+import {Shape, Member} from './protocol'
+


Please add documentation blocks for all interfaces defined.

chrisradek

Overall I think a good first stab. I think your logger is just a little too 'smart', and we need to be more explicit about what the customer needs to provide to it.

chrisradek · 2017-11-15T00:57:24Z

packages/types/src/logger.ts

+}
+
+export interface LoggerOption {
+    logger?: object;


I think we should define an interface to use here beyond object.

I think we have 2 choices:

Allow the customer to provide an object with a defined method that accepts input, like log or write.

Allow the customer to provide an object with defined methods that roughly correlate to the log level mapping.

Option 1 could look something like:

export interface outputter { write: (content: string) => void; }

Option 2 could look something like:

export type outputFn = (content: string) => void; export interface outputter { log: outputFn; info: outputFn; warn: outputFn; error: outputFn; }

Right now with the way the Logger is written, the compiler won't give them any info, and they'll have to find out why their logs don't appear when their application is running (assuming they provide an invalid object).

We could even get real fancy and support both by doing something like prepending the log level to the front on the string if they provided a method that just supported write!

Right now I'd lean towards option 2 because then customers can do more advanced things like log errors to a different file, and get color coding when using console.

Here I believe using Logger interface in replacement of object will be sufficient

chrisradek · 2017-11-15T00:59:09Z

packages/types/src/logger.ts

+    Error = 'error'
+}
+
+export interface LoggerOption {


Future: Might be nice to have options to enable timestamps and log-level logging on anything that's logged:
YYYY-MM-DDTHH:MM:SSZ [level] content

chrisradek · 2017-11-15T01:07:13Z

packages/types/src/logger.ts

+    info(content: string): void;
+}
+
+export interface paramsOperation {


I know we haven't been great about comments, but can you add jsdocs for the interfaces you are adding? Right now I'm not sure by looking at it what paramsOperation is used for. Is paramsOperation the right name to be using as well?

chrisradek · 2017-11-15T01:19:48Z

packages/remove-sensitive-logs/src/index.ts

+        return undefined
+    }
+    if (member.sensitive) {
+        return '******';


Nit: this suggests we use the string <**-redacted-**> for sensitive fields. I don't know what other SDKs use, but this is what services should return.

chrisradek · 2017-11-15T01:26:38Z

packages/remove-sensitive-logs/src/index.ts

+    return mapObjToShape(obj, member.shape);
+}
+
+export const removeSensitiveLogs: paramsOperation = function (obj: any, member: Member): string {


As a generic package, I would actually expect this to return an object that's similar to what was passed in as obj, just with sensitive traits replaced with the redacted string (or something similar). Returning a stringified JSON seems to specific to the specific problem we're trying to solve. (Plus, if I want to stringify it with new lines and tabs, I'd have to JSON.parse and then JSON.stringify again.)

chrisradek · 2017-11-15T01:32:10Z

packages/middleware-operation-logging/src/index.ts

+        const {logger} = this.context;
+        const StartTime = Date.now();
+        return this.next.handle(args).then(output => {          
+            if (logger.logOperationInfo) {


Since this middleware's purpose is to log operation info, I don't think it's necessary to have an extra check in the logger to determine if this info should be logged. The middleware shouldn't care about the logger at all except that it will accept it's content.

If the customer doesn't want this info logged, but does want other info logged, they could always remove this middleware from their stack.

chrisradek · 2017-11-15T01:32:52Z

packages/middleware-operation-logging/src/index.ts

+    handle(args: HandlerArguments<any>): Promise<any> {
+        const {input} = args;
+        const {logger} = this.context;
+        const StartTime = Date.now();


Nit: only constructors (and interfaces) should start with a capital letter.

new commits pushed

* add logger middleware and logger class * update logger level setting

lock · 2019-09-26T21:07:22Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

AllanZhengYP added 6 commits November 2, 2017 00:00

add logger middleware and logger class

a7cbab5

implement remove-sensitive-logs package

1b73b34

add unit test for logger and middleware

7fcbf81

sync with master

23e0a60

edit package.json

b6d50f5

impliment Formatter; update unit test

95729e3

jeskew reviewed Nov 9, 2017

View reviewed changes

AllanZhengYP added 6 commits November 9, 2017 17:13

update logger level setting

ffd6ca8

change remove-sensitive-log output to string

2c96818

remove log formatter

8487059

some reformat

ba7ddbe

rename interface Writablelogger to Logger

44ae6d0

add logOperationInfo flag to enable logging middleware

908b34e

AllanZhengYP changed the title ~~[WIP] Logger & Formatter & LoggerMiddleware~~ Logger & LoggerMiddleware Nov 14, 2017

update according to CR

bf2d520

jeskew reviewed Nov 15, 2017

View reviewed changes

chrisradek previously requested changes Nov 15, 2017

View reviewed changes

update addressing the reviews

28286ae

AllanZhengYP merged commit 13aebe9 into aws:master Nov 18, 2017

trivikr referenced this pull request in trivikr/aws-sdk-js-v3 Dec 10, 2018

Logger & LoggerMiddleware (#66)

638f2a1

* add logger middleware and logger class * update logger level setting

lock bot locked as resolved and limited conversation to collaborators Sep 26, 2019


		export type LogLevel = 'info' \| 'log' \| 'warn' \| 'error';

		export namespace Logging {

		@@ -0,0 +1,27 @@
		import {Shape, Member} from './protocol'

		export enum LogLevel {

Logger & LoggerMiddleware #66

Logger & LoggerMiddleware #66

Uh oh!

Conversation

AllanZhengYP commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lock bot commented Sep 26, 2019

Uh oh!

Uh oh!

AllanZhengYP commented Nov 9, 2017 •

edited

Loading