Allow setting custom_tags for traces #4181

psalaberria002 · 2022-04-01T13:06:44Z

It allows setting trace tags based on literal values, environment variables, or request headers.

Based on https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/tracing/v3/custom_tag.proto#custom-tag

tag_headers should be deprecated in favor of custom_tags. I haven't worked with CRDs and field deprecations. What's the procedure?

When it comes to updating the documentation, where is that handled? I cannot find any reference to TracingService inside the docs folder.

psalaberria002 · 2022-04-07T07:02:15Z

I have experimented with a couple of alternatives, and I am not sure which one if preferred.

Option 1: Custom conversions

When storing from v3alpha1 into v2, convert DeprecatedTagHeaders into custom tags and append to CustomTags,
and store as v2 v3CustomTags.

When reading back from v2 to v3alpha1, convert TagHeaders into custom tags and append to v3CustomTags,
then copy that into v3alpha1 CustomTags.

Whatever you put in v3alpha1 goes cleanly into v2, but when converting it back again to v3alpha1 the objectdoes not have the exact same data. The v3alpha1 DeprecatedTagHeaders will always be empty.

Option 2: No need for custom conversions

v2 TagHeaders <-> v3alpha1 DeprecatedTagHeaders
v2 V3CustomTags <-> v3alpha1 CustomTags

Whatever you put in v3alpha1 goes cleanly into v2, and when converting it back again to v3alpha1 the object has the exact same data. The TestConvert fn in conversion_test.go assumes this behavior.

kflynn · 2022-05-09T14:51:42Z

Hey @psalaberria002! As I understand it:

Any TagHeader can be expressed as a CustomHeader, but not the other way around; and
Transforming a TagHeader to a CustomHeader is trivial and mechanical.

That implies to me that the best answer here is:

v3alpha1 should consider TagHeader to be deprecated
- if only CustomHeader is set, great
- if only TagHeader is set, translate to CustomHeader
- if both TagHeader and CustomHeader are set, log a warning, ignore TagHeader, and use CustomHeader
- you should rename the TagHeader field internally to DeprecatedTagHeader, but leave the JSON tag as TagHeader
v2 should gain a V3CustomHeader field, used only for translations
When translating from v3alpha1 to v2:
- the v3alpha1 CustomHeader field should be saved unaltered as the v2 V3CustomHeader field
When translating from v2 to v3alpha1:
- if v2 V3CustomHeader is set, it should be copied unaltered to v3alpha1 CustomHeader
- otherwise, if v2 TagHeader is set, it should be translated to v3alpha1 CustomHeader
- the translation code must never set v3alpha1 DeprecatedTagHeader
In v3 final, the DeprecatedTagHeader field will go away.

How does that sound?

psalaberria002 · 2022-05-09T19:05:44Z

Hey @psalaberria002! As I understand it:

Any TagHeader can be expressed as a CustomHeader, but not the other way around; and

Transforming a TagHeader to a CustomHeader is trivial and mechanical.

That implies to me that the best answer here is:

v3alpha1 should consider TagHeader to be deprecated

if only CustomHeader is set, great

if only TagHeader is set, translate to CustomHeader

if both TagHeader and CustomHeader are set, log a warning, ignore TagHeader, and use CustomHeader

you should rename the TagHeader field internally to DeprecatedTagHeader, but leave the JSON tag as TagHeader

v2 should gain a V3CustomHeader field, used only for translations

When translating from v3alpha1 to v2:

the v3alpha1 CustomHeader field should be saved unaltered as the v2 V3CustomHeader field

When translating from v2 to v3alpha1:

if v2 V3CustomHeader is set, it should be copied unaltered to v3alpha1 CustomHeader

otherwise, if v2 TagHeader is set, it should be translated to v3alpha1 CustomHeader

the translation code must never set v3alpha1 DeprecatedTagHeader

In v3 final, the DeprecatedTagHeader field will go away.

How does that sound?

Makes sense. I have tried to follow that approach in the last commit. It would be nice to have CI for this PR :)

LanceEa · 2022-06-07T22:06:40Z

@psalaberria002 - Thanks for being patient on this. With the recent release master has diverged quite a bit. Can you please rebase this back on master and push it. I will then get CI running on it.

Note: it's probably good to run git fetch origin --prune to ensure you are not holding onto any old refs for master before re- pulling it. I had that issue ☹️ .

psalaberria002 · 2022-06-07T22:09:53Z

Already pulled from master, cherry picked my change, and force pushed.

LanceEa

@psalaberria002 - it looks pretty good and seems to line up with the discussion between Flynn and you.

I have a few open items I want to check with the team to make sure I understand historical context and a left a few nits as well.

We may want a pytest/kat test for when both req_hdrs and custom_tags but let me find out more on whether that is necessary.

pkg/api/getambassador.io/v2/handwritten.conversion.go

pkg/api/getambassador.io/v3alpha1/crd_tracingservice.go

python/ambassador/envoy/v3/v3listener.py

python/ambassador/ir/irtracing.py

LanceEa · 2022-07-05T19:02:26Z

@psalaberria002 - lets get this rebased on master and update the release notes so that it will land on the upcoming 3.1.0 release. Once that is done I will pull it in and get CI running to make sure all is still green. I want to get a second review from one of the teammates as well.

Again thanks for the patience on this.

psalaberria002 · 2022-08-04T10:57:04Z

@psalaberria002 - lets get this rebased on master and update the release notes so that it will land on the upcoming 3.1.0 release. Once that is done I will pull it in and get CI running to make sure all is still green. I want to get a second review from one of the teammates as well.

Again thanks for the patience on this.

Rebased. I am not able to run tests locally. It would be great if you could run this in CI.

LanceEa

A couple final last nits and then I'm all good. If you could make those changes and then I will pull them in, verify CI is still green and we can get this merged!

Thanks for all the help and putting up with the painful process!

docs/releaseNotes.yml

pkg/api/getambassador.io/v2/handwritten.conversion.go

LanceEa · 2022-08-16T21:17:48Z

pkg/api/getambassador.io/v3alpha1/crd_tracingservice.go

-	StatsName  string         `json:"stats_name,omitempty"`
+	Service  string         `json:"service,omitempty"`
+	Sampling *TraceSampling `json:"sampling,omitempty"`
+	// tag_headers is deprecated. Use custom_tags instead.


It looks kubebuilder has no notion of deprecation validation so let's at least take advantage of Golangs doc comments for deprecated.

https://stackoverflow.com/questions/7849663/how-do-you-mark-code-as-deprecated-in-go

This one is a little bit tricky. The comments endup in the CRD specs. The Deprecated convention in Go is for the struct fields, not the json fields. I am not sure if this is worth it. If so, can you write down here the expected comment string?

Correct, the reason I suggested the Go Doc comment is that these objects can be used in the future where we have a transformation engine that is written in Go. This way if objects are being instantiated via Go code the IDE is going to yell you.

However, I was reading through the docs this morning and it looks like there is a deprecated special marker. It says on-type so it might not work on fields but ideally it can be applied to a field too.
https://book.kubebuilder.io/reference/markers/crd.html

+kubebuilder:deprecatedversion doesn't seem to do anything on fields. I am gonna add the Go deprecation comment and use the json fields in the description since those are visible both in the CRD schema and the Go code.

It allows setting trace tags based on literal values, environment variables, or request headers. Based on https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/tracing/v3/custom_tag.proto#custom-tag Signed-off-by: Paul Salaberria <[email protected]>

Signed-off-by: Paul Salaberria <[email protected]>

Simplification in listener v3 to be tested. Signed-off-by: Paul Salaberria <[email protected]>

Signed-off-by: Paul Salaberria <[email protected]>

LanceEa · 2022-09-21T17:44:09Z

@psalaberria002 - Landed this in #4268 per our discussion in slack. Closing this one out.

psalaberria002 force-pushed the custom-tags branch 2 times, most recently from 8543df2 to c89902e Compare April 1, 2022 13:16

LukeShu requested review from LukeShu and removed request for LukeShu April 19, 2022 19:42

psalaberria002 force-pushed the custom-tags branch 4 times, most recently from e8da8d9 to 3a8684f Compare May 9, 2022 19:02

psalaberria002 force-pushed the custom-tags branch from 3a8684f to 97ee0c7 Compare May 9, 2022 19:31

kflynn mentioned this pull request May 18, 2022

CI for #4181 #4236

Open

psalaberria002 force-pushed the custom-tags branch 2 times, most recently from 5835a4b to a8a49ae Compare May 27, 2022 07:50

psalaberria002 force-pushed the custom-tags branch from a8a49ae to c42c9d9 Compare June 6, 2022 19:36

psalaberria002 force-pushed the custom-tags branch from c42c9d9 to af92682 Compare June 7, 2022 22:30

LanceEa suggested changes Jun 8, 2022

View reviewed changes

LanceEa mentioned this pull request Jun 8, 2022

tracing: add new custom_tags field to TracingService #4268

Merged

5 tasks

psalaberria002 force-pushed the custom-tags branch 2 times, most recently from 60edbbb to 8dbe260 Compare June 9, 2022 11:47

psalaberria002 force-pushed the custom-tags branch from cbd0e76 to 755ce83 Compare June 16, 2022 19:56

psalaberria002 force-pushed the custom-tags branch from 755ce83 to 2ee0cad Compare August 4, 2022 10:56

LanceEa reviewed Aug 16, 2022

View reviewed changes

psalaberria002 force-pushed the custom-tags branch from b4359bb to 748eec0 Compare August 17, 2022 11:32

psalaberria002 added 9 commits August 18, 2022 08:06

Reduce nesting

291f93e

Signed-off-by: Paul Salaberria <[email protected]>

Fix nits

0b1f404

Simplification in listener v3 to be tested. Signed-off-by: Paul Salaberria <[email protected]>

Generate

651457b

Signed-off-by: Paul Salaberria <[email protected]>

Fix setting of custom tags after rebase

c5751eb

Signed-off-by: Paul Salaberria <[email protected]>

Fix linting

57c1c2e

Signed-off-by: Paul Salaberria <[email protected]>

Revert unchanged file

341acfb

Signed-off-by: Paul Salaberria <[email protected]>

Remove envoy version check

3b52f8f

Signed-off-by: Paul Salaberria <[email protected]>

Use dlog

2df8649

Signed-off-by: Paul Salaberria <[email protected]>

Add deprecation

62c6438

Signed-off-by: Paul Salaberria <[email protected]>

LanceEa force-pushed the custom-tags branch from f64785b to 62c6438 Compare August 18, 2022 14:21

LanceEa closed this Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting custom_tags for traces #4181

Allow setting custom_tags for traces #4181

psalaberria002 commented Apr 1, 2022 •

edited

Loading

psalaberria002 commented Apr 7, 2022

kflynn commented May 9, 2022

psalaberria002 commented May 9, 2022

LanceEa commented Jun 7, 2022

psalaberria002 commented Jun 7, 2022

LanceEa left a comment

LanceEa commented Jul 5, 2022

psalaberria002 commented Aug 4, 2022

LanceEa left a comment

LanceEa Aug 16, 2022

psalaberria002 Aug 17, 2022

LanceEa Aug 17, 2022

psalaberria002 Aug 18, 2022

LanceEa commented Sep 21, 2022

Allow setting custom_tags for traces #4181

Allow setting custom_tags for traces #4181

Conversation

psalaberria002 commented Apr 1, 2022 • edited Loading

psalaberria002 commented Apr 7, 2022

Option 1: Custom conversions

Option 2: No need for custom conversions

kflynn commented May 9, 2022

psalaberria002 commented May 9, 2022

LanceEa commented Jun 7, 2022

psalaberria002 commented Jun 7, 2022

LanceEa left a comment

Choose a reason for hiding this comment

LanceEa commented Jul 5, 2022

psalaberria002 commented Aug 4, 2022

LanceEa left a comment

Choose a reason for hiding this comment

LanceEa Aug 16, 2022

Choose a reason for hiding this comment

psalaberria002 Aug 17, 2022

Choose a reason for hiding this comment

LanceEa Aug 17, 2022

Choose a reason for hiding this comment

psalaberria002 Aug 18, 2022

Choose a reason for hiding this comment

LanceEa commented Sep 21, 2022

psalaberria002 commented Apr 1, 2022 •

edited

Loading