Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

Refactor to clean up code, better tests, add transaction ID support #4

Merged
merged 4 commits into from
Jun 19, 2016

Conversation

jraede
Copy link
Contributor

@jraede jraede commented Jun 17, 2016

This refactor does a few things:

  1. Clean up the code. It was hard to follow and very opinionated.
  2. Add support for multiple loggers. This allows us to create a logger per transaction that automatically adds transaction ID
  3. Sets up JSON logging for everything. This allows logs to be parsed by logstash, etc.

#### `fatalf(format, ...args)`

### `transactionLogger`
Get a logger that adds a `transaction_id` property to the meta. Useful for tracking related requests.
Copy link

@taddison92 taddison92 Jun 17, 2016

Choose a reason for hiding this comment

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

Should this be the same logger as the other logger and just allow the other logger to accept an optional transaction id? That would make it such that we could just in places we use the same logger everywhere and just add in this parameter in order to make the transaction switch a bit easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but then we run into race conditions if we have code executing in parallel. Imagine this situation:

  1. Execute concurrent transactions using Promise.all
  2. In one, set the transaction ID
  3. If it's a shared instance, then the other chain would get that transaction ID, or overwrite it when it tries to set it for itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other option is providing the transaction ID to every call to log, info, debug, etc, but that would be tedious.

Copy link

@taddison92 taddison92 Jun 17, 2016

Choose a reason for hiding this comment

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

hmm I see what you mean, sort of.

It would be nice to have our services such that on original invocation of the service, set up the logger with the transaction id as a synchronous process, then later whatever operations it does use that logger that has the transaction id already set up (either created or received via parameter). This shouldn't be too hard given that our services use api endpoints as their entry points.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah exactly. I'd like to instantiate a logger using getTransactionLogger on each request and wrap the request in an object that can do like this.log. So this is to help us get towards that.

@taddison92
Copy link

Looks good. I think having the transactionLogger be separate from the regular logger is less preferable than adding to logger, the ability to take an optional parameter.

@jraede jraede merged commit 69996ce into master Jun 19, 2016
@jraede jraede deleted the task/refactor-for-transaction-ids branch June 19, 2016 22:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants