Include response headers in logs

[deadlycoconuts]

Context

In PR #176, a change was introduced to merge the headers of the original request sent to Turing routers with that of the response returned from an enricher. The change in this PR complements the change previously made by introducing logging of the response headers of each response received by a Turing Router.

Modifications

• engines/router/missionctl/handlers/logutils.go - Addition of a step to turn response headers into strings to be logged
• engines/router/missionctl/log/resultlog/proto/turing/TuringResultLog.pb.go - Changes to the generated proto classes
• engines/router/missionctl/log/resultlog/proto/turing/TuringResultLog.proto - Changes to the proto to include response headers
• engines/router/missionctl/log/resultlog/resultlog.go - Changes to the AddResponse method to include response headers as an argument

[deadlycoconuts]

Renamed response as body because the addition of the new field header would create some ambiguity as to what response would really mean.

[krithika369]

That's a valid point. But unfortunately, this will be a breaking change, not for the proto decoding itself, but rather, the downstream workflows that depend on it. For example, in most internal use cases, the Kafka messages are forwarded to BQ and the field name change would result in a change in the table as well.

Similarly, our own BQ logger uses the schema generated from this proto. This means, the next time users update their routers, they have to carry out a schema migration on the existing table as the only thing we handle automatically is addition of new columns (ref).

We can look into a schema migration if required (which we have done once in the past) but it would need the users to take actions to migrate their existing data / scripts as well. So we should avoid it unless critical.

Considering all of the above, I think header, response is not too outlandish and we can live with it.

[krithika369]

Thanks for this PR. It is marked as Draft but I took a quick look anyway and left some comments.

The changes themselves look good. The biggest factor to consider is, how they would play with the existing logging set up - both BQ and Kafka. For example, if someone has a router deployed that's logging to BQ and they update the router, is the new field added to each of the components' records and are they written correctly? These are unit-tested but would be nice to carry out some e2e test as well before merging this PR, locally.

[krithika369]

That's a valid point. But unfortunately, this will be a breaking change, not for the proto decoding itself, but rather, the downstream workflows that depend on it. For example, in most internal use cases, the Kafka messages are forwarded to BQ and the field name change would result in a change in the table as well.

Similarly, our own BQ logger uses the schema generated from this proto. This means, the next time users update their routers, they have to carry out a schema migration on the existing table as the only thing we handle automatically is addition of new columns (ref).

We can look into a schema migration if required (which we have done once in the past) but it would need the users to take actions to migrate their existing data / scripts as well. So we should avoid it unless critical.

Considering all of the above, I think header, response is not too outlandish and we can live with it.

[krithika369]

Could you elaborate more on this non-deterministic behavior? What changes? And why specifically Content-Encoding ?

[deadlycoconuts]

It's essentially when the Header field of turingLogEntry, which is an (unordered) map of strings, get encoded, the order in which the items in Header get serialised is not guaranteed to always be the same, leading to some unit tests failing and not others. Since the default test log entry Header has 2 keys, one for Content-Encoding and the other for Content-Type, I simply removed removed one of them to ensure that the output's always the same.

Another option is to create another test log entry for this kafka test without using makeTestTuringResultLogEntry, or to make makeTestTuringResultLogEntry use only 1 item for the Header field, which would also simplify the other tests using it.

[krithika369]

I see.. In this case, what do you think about decoding the message using the proto and then using assert.JSONEq (as we do in TestNewJSONKafkaLogEntry) to compare?

[deadlycoconuts]

Hmm that's possible, but I'm not sure if the intention of the TestNewProtobufKafkaLogEntry test was to test the byte encoding of the kafka log entry, as opposed to TestNewJSONKafkaLogEntry which seems to be testing for the JSON output. Would using assert.JSONEq defeat the purpose of the other test?

Edit: Okay maybe not, it's do-able I guess.

[deadlycoconuts]

I've added these additional lines after realising that the headers in the response need to be specially handled before being pushed to BigQuery tables. There's a similar step done for the request headers above.

[krithika369]

LGTM! 🎉

[deadlycoconuts]

Thanks for the comments! Merging it now! 🚀