Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backend Challenge Solution - Manan Habib #23

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

manan-habib
Copy link

No description provided.

Copy link
Contributor

@marcelino-sf marcelino-sf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mananhabib31 for taking your time and working on this code challenge. I left a few questions to understand your thinking process and to get your thoughts.

@@ -0,0 +1,101 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason you decided to push the coverage folder?

Copy link
Author

@manan-habib manan-habib Aug 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no major reason. I just had one thought to let reviewer quickly go over the html report(./coverage/lcov-report/index.html) that was generated before my push.

@@ -0,0 +1,68 @@
variable "graphql_api_name" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we grow the project with dozen of variables, and a lot more infrastructure and modules, would it be nice to have them defined in main.tf?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope it wouldn't, if the scope of the project gets bigger and we need more modules and variables to define then we should be adding separate files for each module and variables. If variables grow more then variable file can be broken down on the basis of cohesion of variables. I didn't create a separate one as in the scope of this project I didn't think that we need to.

"reflect-metadata": "^0.1.13",
"ts-node": "^10.9.1",
"typedi": "^0.10.0",
"typescript": "^4.7.4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see some developer dependencies defined in this section, what was the reason you decided to add them as production dependencies?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right. That's a mistake from my side. I should have used -D or --save-dev while installing these. My bad!

@@ -0,0 +1,27 @@
import dotenv from 'dotenv';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the reason you wanted to have dotenv in production w/ lambdas?

Copy link
Author

@manan-habib manan-habib Aug 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular thing I have inherited from my .net core experience. In .net core, developers prefer to have appsettings file and one can even override the env variable by using appsettings json file. It's just more continent to use settings file because .net framework itself supports better maintainability of by allowing them to be broken down on cohesion basis.We are using the same thing in our identity server app as well. But seeing your comment on this thing making me realize things are different in nodejs. In simple words, it's just due to past experience.

aws.env Outdated
NODE_ENV="aws"

# Google Geocode API
GOOGLE_MAPS_KEY='AIzaSyBj1aLo0X1uH7CKoi3bXPFPdX6eeCra4e8'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you want to push this API key to the repo? This is a sensitive value

I encourage you to remove it as quickly as possible from your google console before someone malicious start using it causing undexpected charges on your credit card

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well you are right that its a sensitive info and the only motivation behind adding this was just to let reviewer instantly test the functionality(locally) without configuring its own key. To be on safe side, I already have a cap on budget of 5$ monthly to avoid any surprise situation.

GOOGLE_MAPS_KEY='AIzaSyBj1aLo0X1uH7CKoi3bXPFPdX6eeCra4e8'

# Debug
LOG_LEVEL='error'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help me understand why you added just error logs to the AWS lambda in production. What is your opinion about INFO, WARN, DEBUG, and TRACE logs in production?

Copy link
Author

@manan-habib manan-habib Aug 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well there was no specific reason to set level to 'error' in aws env file. I was just testing all the levels with cloudwatch and ended up on error. To answer your question that what should we be doing in production then under normal circumstances production log level should be 'Info'. As far as the different log levels are concerned:

Info: These logs gives useful information about the system in general. This is sort of information which one don't really care about under normal circumstances but very helpful in crisis situation.
WARN: Gives you info about anything that has a potential to cause error OR fatal exception. Usually we have mechanisms for auto-recovery(or system gets recovered on its own sometimes) with problems logged on this level but this keeps us alerted that what can go wrong in future.
Debug/Trace: I would like to address debug and trace in same part. There is always a debate between level of these log levels in hierarchy but in my experience, debug < trace. This is the level which we only turn on in the situation of diagnoses. If we consider debug < trace side, then debug logs fine grained information(but less granular than Trace) about operation being performed while trace level gives us the most fine grained info(even step by step sometimes) about the operation. We only enable it to diagnose the things in production because it has a performance impact on system.

Note: I assumed that we are discussing log TRACE level and not distributed tracing in terms of microservices.

@@ -0,0 +1,38 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to have a separate package.json file for linux?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I developed this project on a windows machine and some commands in package file are windows based e.g. copy file command. So after completing my work I spinned up a linux machine and created another working package file for reviewer(assuming that reviewer would have linux based system).

}
} catch (error) {
/* istanbul ignore next */
this.logger.error(`locationService.getCoordinates ended up in exception. Error: ${error}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this error being exposed to the client somehow? I meean, when there's an error in this service, do the client know what happened or they just receive an arbitrary message?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't end up to send arbitrary message but a null object in response which you are right about that it should be sending an error. I made a mistake there and overlooked this place to return error object.


test('Should return error for empty string value', async () => {
try {
var result = NonEmptyString.serialize('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see by the title that the test is expecting an error, but if this line doesn't throw anything it will pass the test anyway

Copy link
Author

@manan-habib manan-habib Aug 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right I skipped that case where if someone stop throwing the error from subject method then it will pass anyway. I should have used following instead of current expect statement:
expect(NonEmptyString.serialize('')).toThrowError({message: Value can't be empty, name: 'VALUE_NOT_EMPTY'});

@@ -0,0 +1,5 @@
export class CoordinatesQuery {
public async getCoordinates(parent: any, args: any, {dataSources}: any, info: any){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you want to use any instead of actual types? Wouldn't using types better since we're using TypeScript?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I didn't specify types there is that this resolver just wire down the request to location service and do nothing else significant but yeah in context of typescript usage specifying types would make more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants