Add envoy-json-logging-custom-fields-design doc #3042

mike1808 · 2020-10-19T22:08:01Z

Co-authored-by: Leah Hanson lhanson@pivotal.io

In continuation of #3032 we're making a design doc to discuss possible solutions for our problem.

codecov · 2020-10-19T22:12:06Z

Codecov Report

Merging #3042 into main will decrease coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3042      +/-   ##
==========================================
- Coverage   73.35%   73.22%   -0.14%     
==========================================
  Files          95       95              
  Lines        6046     6057      +11     
==========================================
  Hits         4435     4435              
- Misses       1510     1521      +11     
  Partials      101      101

Impacted Files	Coverage Δ
cmd/contour/serve.go	`0.00% <0.00%> (ø)`

jpeach · 2020-10-19T22:41:00Z

design/envoy-json-logging-custom-fields-design.md

+  - This is related to [#2523](https://github.com/projectcontour/contour/issues/2523)
+* Is one of the alternative approaches preferable?
+   - [User-defined Translation Table](#user-defined-translation-table) seems easier to implement because it was partially implemented in #3033 and because the configuration is more structured and easier to work with as there no need for additional string parsing.
+   - [Map Syntax](#map-syntax) is not backward-compatible, but does avoid string parsing.


Map syntax seems clean, but we would need to introduce it in a new config field, then deprecate the old field after a number of releases. Would still need to define what happens to the built-in field mappings.

Yea, since the map syntax would require deprecating the prior configuration field anyway, the new field would not have to support the built-in field mappings. In our example in this doc, we assumed that empty values would imply that the key should be looked up in the built-in field mappings, but I'm not sure whether that's desirable or not.

In favor of a built-in translation table:

Users of Contour only have to read Contour's docs (vs. also referencing Envoy's config syntax).

Users get reasonable, standardized field names because that's what's setup by the field mappings.

In favor of writing all key-value pairs out:

Users of Contour can read the Envoy docs to decide what config to write. They don't need to know Contour's specific translation of special keys.

I like the ease of use of having the translation table, personally. I think that having the two types seems like an okay tradeoff for that ease of use (for now at least).

jpeach

This looks good to me. I'll give other maintainers a chance to review before I merge though.

jpeach · 2020-10-19T22:45:37Z

design/envoy-json-logging-custom-fields-design.md

+  - In the current implementation, `method` becomes `"method": "%REQ(:METHOD)%"`.
+  - When the new implementation adds a way to achieve the JSON config `"response_duration": "%RESPONSE_DURATION%"`, should the user write:
+    * `response_duration`, or
+    * `response_durationn=%RESPONSE_DURATION%`


The built-in mappings still need to be supported. I would not want to force people using the existing field mappings to manually remake their configuration.

We agree that built-in mappings need to be supported. The question here is whether fields that could be added to those mappings should be.

method will still work.

should response_duration work?

If response_duration works, then the interface is consistent, but the implication is that the translation table will be used forever (despite users having a way to write response_duration without the translation table).
If response_duration=%RESPONSE_DURATION% is the only option for users, then there is inconsistency between the original set that are in the translations and the new ones that require more spelling out.

The original intent was that we add simple translations as required to the translation table; we figured that people could just request anything they needed added. With that in mind, I'm okay with adding whatever simple translations to the translation table (like response_duration, where it's a straight mapping between field name and Envoy template string), as part of this implementation.

Obviously, this will be frozen at this point in time, but perhaps a comment saying that adding things to the standard translations is fine would make it clearer that we're open to adding more?

jpeach · 2020-10-19T22:49:05Z

design/envoy-json-logging-custom-fields-design.md

+
+#### Unknown field name in old format
+
+If the field name is not defined in the default fields transition map and the user doesn't specify the Envoy field format, then Contour will raise a validation error.


See #3023 for config file validation.

youngnick

This proposal LGTM, I think it's a good balance between adding extra functionality and backward compatibility. Thanks for writing it up @mike1808 and @astrieanna!

Co-authored-by: Leah Hanson <lhanson@pivotal.io> Co-authored-by: Mikael Manukyan <mmanukyan@vmware.com> Signed-off-by: Mikael Manukyan <mmanukyan@vmware.com>

Co-authored-by: Leah Hanson <lhanson@pivotal.io> Signed-off-by: Mikael Manukyan <mmanukyan@vmware.com>

stevesloka · 2020-10-20T17:22:38Z

design/envoy-json-logging-custom-fields-design.md

+
+#### Field name is repeated
+
+If the field name is repeated the last entry in the configuration takes precedes and overwrites the previous value.


Wonder if this should raise a validation error instead of overwriting?

A validation error could be OK if a field name is repeated in a single json-fields array. My original intention with suggesting overwriting was that operators should be able to use this mechanism to change the definition of builtin log fields (ie. overwriting the builtin field mapping).

skriss

LGTM!

jpeach · 2020-10-20T22:27:52Z

Last call for comments.

youngnick · 2020-10-21T23:12:54Z

design/envoy-json-logging-custom-fields-design.md

@@ -0,0 +1,144 @@
+# Allow Envoy to output arbitrary fields for JSON logs
+
+Status: Draft


If there are no further comments, @mike1808, could you change this to 'Accepted' before we merge?

Done @youngnick

Co-authored-by: Mikael Manukyan <mmanukyan@vmware.com>

youngnick · 2020-10-22T00:28:35Z

Okay, I'm calling this one done. Thanks @mike1808 and @astrieanna!

jpeach added area/deployment Issues or PRs related to deployment tooling or infrastructure. kind/design Categorizes issue or PR as related to design. labels Oct 19, 2020

jpeach added this to In progress in Contour Project Board via automation Oct 19, 2020

jpeach reviewed Oct 19, 2020

View reviewed changes

jpeach approved these changes Oct 19, 2020

View reviewed changes

youngnick approved these changes Oct 19, 2020

View reviewed changes

mike1808 force-pushed the envoy-custom-json-fields-design branch from 921cec7 to 43602ae Compare October 19, 2020 23:58

mike1808 and others added 2 commits October 19, 2020 23:59

Add envoy-json-logging-custom-fields-design doc

eefcb33

Co-authored-by: Leah Hanson <lhanson@pivotal.io> Co-authored-by: Mikael Manukyan <mmanukyan@vmware.com> Signed-off-by: Mikael Manukyan <mmanukyan@vmware.com>

Fix review comments

1b6c9ec

Co-authored-by: Leah Hanson <lhanson@pivotal.io> Signed-off-by: Mikael Manukyan <mmanukyan@vmware.com>

mike1808 force-pushed the envoy-custom-json-fields-design branch from 43602ae to 1b6c9ec Compare October 19, 2020 23:59

Merge branch 'main' into envoy-custom-json-fields-design

0e142d2

stevesloka reviewed Oct 20, 2020

View reviewed changes

skriss approved these changes Oct 20, 2020

View reviewed changes

youngnick reviewed Oct 21, 2020

View reviewed changes

Accept envoy-json-logging-custom-fields-design

8d743ae

Co-authored-by: Mikael Manukyan <mmanukyan@vmware.com>

youngnick merged commit c9563ed into projectcontour:main Oct 22, 2020

Contour Project Board automation moved this from In progress to 1.10 Release Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add envoy-json-logging-custom-fields-design doc #3042

Add envoy-json-logging-custom-fields-design doc #3042

mike1808 commented Oct 19, 2020

codecov bot commented Oct 19, 2020 •

edited

jpeach Oct 19, 2020

astrieanna Oct 19, 2020

youngnick Oct 19, 2020

jpeach left a comment

jpeach Oct 19, 2020

astrieanna Oct 19, 2020

youngnick Oct 19, 2020

jpeach Oct 19, 2020

youngnick left a comment •

edited

stevesloka Oct 20, 2020

jpeach Oct 20, 2020

skriss left a comment

jpeach commented Oct 20, 2020

youngnick Oct 21, 2020

mike1808 Oct 21, 2020

youngnick commented Oct 22, 2020


		#### Unknown field name in old format

		If the field name is not defined in the default fields transition map and the user doesn't specify the Envoy field format, then Contour will raise a validation error.


		#### Field name is repeated

		If the field name is repeated the last entry in the configuration takes precedes and overwrites the previous value.

		@@ -0,0 +1,144 @@
		# Allow Envoy to output arbitrary fields for JSON logs

		Status: Draft

Add envoy-json-logging-custom-fields-design doc #3042

Add envoy-json-logging-custom-fields-design doc #3042

Conversation

mike1808 commented Oct 19, 2020

codecov bot commented Oct 19, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

In favor of a built-in translation table:

In favor of writing all key-value pairs out:

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

jpeach commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick commented Oct 22, 2020

codecov bot commented Oct 19, 2020 •

edited

youngnick left a comment •

edited