JCN-112-Log

[Nataniel4]

JCN-112-LOG
JCN-112 Crear paquete log

LINK AL TICKET
https://fizzmod.atlassian.net/browse/JCN-112

DESCRIPCIÓN DEL REQUERIMIENTO
1.1. Se requiere crear un paquete nuevo para la creación de logs
1.2. Los logs se deben crear en el servicio S3
1.3. Las credenciales aws_access_key_id y aws_secret_access_key se deben tomar de variables de entorno y que cada MS las tendrá configuradas.

DESCRIPCIÓN DE LA SOLUCIÓN
Se desarrollo lo solicitado en el ticket

[jormaechea]

Te dejé varios comentarios.
En mi opinión, hiciste código demasiado complejo para una tarea super simple: hacer una request con reintentos.

Con respecto a los tests: hubo un buen motivo para hacer un "mock" de AWS con tanta complejidad? Alcanzaba con hacer algo como

sandbox.stub(AWS, 'S3')
	.returns({
		putObject: someSinonStub
	});

y después assertear que se haya llamado a someSinonStub correctamente.
Hay mucha complejidad innecesaria ahí también

[jormaechea]

Acá no hace falta pasarle las keys. De hecho, cuando se ejecute en lambda no hay keys.
Simplemente no lo pasás, y en el docker-compose.yml se monta el siguiente volumen - ~/.aws:/root/.aws

Ver el MS de router, que ya está implementado de esta manera.

[jormaechea]

PD: Documentar en el README.

[jormaechea]

Perdón, pero qué necesidad de declarar un objeto con una prop y 3 getters, si se va a usar una sola vez, y los getters devuelven exactamente lo mismo cada vez que se los llama?

Es extremadamente más simple, fácil de entender y performante hacer simplemente

const date = log.date_created ? new Date(log.date_created * 1000) : new Date();
const year = date.getFullYear();
const month = (date.getMonth() + 1).toString().padStart(2, 0);
const day = date.getDate().toString()
		.padStart(2, 0);

[jormaechea]

Todos los métodos de los SDK de Amazon se pueden usar con Promise en lugar de callbacks.

Para este ejemplo, podés hacer algo asi:

return S3.putObject({ ... }).promise();

Y del otro lado lo manejás con un await.
(Ver comentario sobre el método add de cómo simplificarlo)

[jormaechea]

Demasiado complejo para una tarea tan simple. Mirá qué simple se puede dejar todo esto, sin definir funciones adentro de funciones, con intervalos cuando ya le pusiste un timeout a la request.

static async add(bucket, log, attempts = 0) {

	try {
		await this._add(bucket, log);
	} catch(err) {
		if(attempts >= MAX_ATTEMPTS)
			throw new LogError(`Unable to put the log into S3, max attempts reached: ${err.message}`, LogError.codes.S3_ERROR);

		return this.add(bucket, log, ++attempts);
	}
}

Por otro lado, en el catch podrías manejar la lógica de no hacer reintentos para los dos error codes INVALID_BUCKET y INVALID_LOG. No tiene sentido intentar guardar un log 3 veces si ya sabés que va a volver a fallar.

[jormaechea]

Suggested change

	const S3 = new AWS.S3({ httpOptions: { timeout: MAX_TIMEOUT } });
	const S3 = new AWS.S3({ httpOptions: { timeout: MAX_TIMEOUT } });

[Nataniel4]

Que raro eso, no sale en el editor

[jormaechea]

Debe coincidir el tab-stop justo en un caracter..

[jormaechea]

Ojo con estos throw.. La aplicación no debería explotar por no poder guardar un log.
Por el momento, simplemente loggearía en la consola que no se pudo guardar el log, y el motivo.

[jormaechea]

Corrijo: no sé si loggear en la consola es una buena decisión en un package.
Quizás sea una mejor decisión emitir un evento de node..

Para eso, deberías crear un event emitter fuera de la clase

const EventEmitter = require('events');
const emitter = new EventEmitter();

Y agregar un método .on() a la clase Log, que haga algo asi

static on(event, callback) {
	emitter.on(event, callback);
}

Y finalmente, emitir el evento:

emitter.emit('create-error', log, err);

---

Si alguien quisiera suscribirse a los errores al crear logs, haría algo asi:

const Log = require('@janiscommerce/log');
Log.on('create-error', (log, err) => {
	// do something
});

[jormaechea]

Este MockRequire creería que no hace falta.
Por otro lado, un par de lineas más abajo estás creando un stub de AWS.S3, y nunca lo estás restoreando. Ojo ahí.

[jormaechea]

4 llamados? El máximo de intentos está setteado como 3..