170 - re-enabled request logger and made compatible with Avro

[mmacai]

Description

Fixed API Gateway's request logger to work with latest Kafka client/interface & added integration with Avro.

Split integration tests into 2 parts - Kafka without Avro & Kafka with Avro -- reason is easier configuration and less issues in tests themselves.

Closes #170
Closes #201

What's missing:

• working integration test for request logger with Avro

What to look out for

• Check docs and try to run RIG with the instructions found there. (might be that it's not informative enough)
• There is an issue with request logger integration test and Avro (the commented one) - it's due to the fact that as RIG starts it spawns producer process for request logger and if I try to change env var ad-hoc in test it can't be overridden (since process already started). - @kevinbader any thoughts?
  • Solved by splitting integration tests into 2 parts - with and without avro -- this should also increase integration tests stability

Process

The goal is to improve not only the code in this PR but also our skills! The "rules":

• The review is considered "done" as soon as all reviewers have added their review, and all their comments have been addressed.
• For knowledge-sharing reviews, each reviewer should "approve" the PR after studying its content.
• After the approval, the merge is concluded by the developer.

Have fun!

[mmacai]

Ready for review.

[kevinbader]

Changes look good for the most part! What could be improved:

• If RIG is started with REQUEST_LOG=asdf, I'd expect RIG to fail on startup. Instead, RIG returns 500 Internal Server Error when hit with a request, without emitting a warning of any kind.
• RIG fails the same way for REQUEST_LOG=, which really should only disable request logging.
• RIG also fails for REQUEST_LOG=kafka in case Kafka is not configured.
  1. RIG should fail on startup if this is set and Kafka is not enabled.
  2. If Kafka is enabled but there's a connection issue, the request logger should emit a warning but the service response should not be affected - the client shouldn't see a 500 just because its request couldn't be logged successfully.

[mmacai]

Changes look good for the most part! What could be improved:

• If RIG is started with REQUEST_LOG=asdf, I'd expect RIG to fail on startup. Instead, RIG returns 500 Internal Server Error when hit with a request, without emitting a warning of any kind.

• RIG fails the same way for REQUEST_LOG=, which really should only disable request logging.

• RIG also fails for REQUEST_LOG=kafka in case Kafka is not configured.

  1. RIG should fail on startup if this is set and Kafka is not enabled.
  2. If Kafka is enabled but there's a connection issue, the request logger should emit a warning but the service response should not be affected - the client shouldn't see a 500 just because its request couldn't be logged successfully.

Hmm, do you think it's necessary to shutdown entire RIG when env var is misconfigured or kafka is not set, but logger (with kafka value) is?

In the first case, I guess it makes sense - Kafka works in a similar way when you misconfigure something.

In the second case, I'm not sure -- warning seems enough for me.

[kevinbader]

Hmm, do you think it's necessary to shutdown entire RIG when env var is misconfigured or kafka is not set, but logger (with kafka value) is?

I wouldn't shut down a running instance, but I would prevent starting a new one with this configuration.

[mmacai]

Hmm, do you think it's necessary to shutdown entire RIG when env var is misconfigured or kafka is not set, but logger (with kafka value) is?

I wouldn't shut down a running instance, but I would prevent starting a new one with this configuration.

Added my first shot, but you can more or less see the direction in git diff.

In general, goal is:

• if REQUEST_LOG is set to something random => exit
• if REQUEST_LOG= => exit
• if REQUEST_LOG=kafka, but KAFKA_BROKERS not set => exit
• if REQUEST_LOG=kafka, KAFKA_BROKERS set, but there are Kafka connection issues => HTTP response should be ok and produce fail
  • changed logging from call to cast, as in my opinion it shouldn't block the HTTP response

[kevinbader]

Please rename RIG.ConfigValidation to something related to the logger. I feel it's better to group validations with the feature they're related to, rather than creating a central module that potentially validates everything.

• if REQUEST_LOG is set to something random => exit

👍

• if REQUEST_LOG= => exit

Above I wrote "RIG fails the same way for REQUEST_LOG=, which really should only disable request logging." - still feel the same way. Why do you prefer exit in this case?

• if REQUEST_LOG=kafka, but KAFKA_BROKERS not set => exit

👍

• if REQUEST_LOG=kafka, KAFKA_BROKERS set, but there are Kafka connection issues => HTTP response should be ok and produce fail

👍 if with "produce fail" you mean "produce warning".

• changed logging from call to cast, as in my opinion it shouldn't block the HTTP response

👍

[mmacai]

Added filtering for env var value that should prevent failing on cases like REQUEST_LOG= or REQUEST_LOG=kafka,.