tracestate: Allow duplicate keys but rewrite them by marclop · Pull Request #1183 · elastic/apm-agent-go

marclop · 2022-01-05T07:32:11Z

Description

Changes the validation logic to allow duplicate keys to be present in
tracestate entries, modifying the NewTraceState constructor to rewrite
the duplicate es keys according to the new W3C spec, but leaving 3rd
party duplicates untouched.

https://w3c.github.io/trace-context/#combined-header-value

Related issues

Closes #1182

Changes the validation logic to allow duplicate keys to be present in tracestate entries, modifying the `NewTraceState` constructor to rewrite the duplicate keys according to the new W3C spec. Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

ghost · 2022-01-05T08:07:37Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-01-10T06:36:55.249+0000
Duration: 34 min 34 sec
Commit: 4344152

Test stats 🧪

Test	Results
Failed	0
Passed	11991
Skipped	279
Total	12270

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run benchmark tests : Run the benchmark test.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

marclop · 2022-01-05T08:46:28Z

tracecontext.go

+	var out TraceState
 	var last *TraceStateEntry
+	var modified []TraceStateEntry
+	recorded := make(map[string]uint8)


This could also be named seen or observed, I think it works either way but I'm happy to rename it.

doesn't make a big difference to me, happy with whichever color you choose for the bikeshed :)

tracecontext.go

tracecontext_test.go

stuartnelson3

couple small comments, but nothing that requires a re-review

stuartnelson3 · 2022-01-05T13:56:51Z

tracecontext.go

+	var out TraceState
 	var last *TraceStateEntry
+	var modified []TraceStateEntry
+	recorded := make(map[string]uint8)


doesn't make a big difference to me, happy with whichever color you choose for the bikeshed :)

stuartnelson3 · 2022-01-05T13:59:40Z

tracecontext.go

+	}
+	// After we've ranged over the whole entry slice, any duplicates present
+	// must be written first in a last to first order, thus we range the slice
+	// backwards.


Ah hah, the order is why modified is a slice and not a map?

stuartnelson3 · 2022-01-05T14:01:15Z

tracecontext.go

+			if count > 1 {
+				for i, m := range modified {
+					if m.Key == e.Key {
+						modified[i] = e


Once we've found a matching key, we could continue, right? Since we're only wanting to record the latest entry e for a specific key e.Key?

yeah we could if we kept the implementation as is, I'm going to simplify it and change it to only act on the 'es' vendor key which is the only one we care about, it'll also eliminate allocations :)

stuartnelson3 · 2022-01-05T14:02:54Z

tracecontext.go

+		recorded[e.Key]++
+	}
+	// After we've ranged over the whole entry slice, any duplicates present
+	// must be written first in a last to first order, thus we range the slice


nit: must be written in a last to first order, I think it makes the comment a bit simpler since we're already saying last and first

tracecontext.go

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

tracecontext.go

tracecontext_test.go

axw · 2022-01-06T09:10:23Z

tracecontext.go

+	var out TraceState
 	var last *TraceStateEntry
+	var recorded uint8
+	// Range over the entries and find duplicate `es` vendor keys. When 'es'


I'm finding this method quite difficult to follow now. Would this alternative work?

var haveElastic bool var last *TraceState for _, e := range entries { if e.Key == elasticTracestateVendorKey { if haveElastic { // Discard duplicate "es" entries; keep the last entry's value. out.head.value = e.value continue } haveElastic = true e.next = last last = nil // cause elastic to be moved to head } e := e // copy if last == nil { out.head = &e } else { last.next = &e } last = &e } if haveElastic { out.parseElasticTracestateError = out.parseElasticTracestate(out.head) }

BTW we didn't previously move the elastic entry to the head because IIANM that's only expected when mutating tracestate.

I mostly kept the suggestion intact, but had to make some changes in 9270b6a to not lose any entries that were found previous to the es key.

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

axw

thanks for the updates!

tracecontext.go

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

Changes the validation logic to allow duplicate keys to be present in tracestate entries, modifying the `NewTraceState` constructor to rewrite the duplicate `es` keys according to the new W3C spec, but leaving 3rd party duplicates untouched. Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

* CI: use apm-integration-testing/7.x for v1 Use apm-integration-testing 7.x branch for testing apm-agent-go v1. We'll update apm-integration-testing's main branch to test v2. * deps: Pin `olivere/elastic` dependency (#1178) To version `v7.0.29` for pre module Go versions. Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com> * tracestate: Allow duplicate keys but rewrite them (#1183) Changes the validation logic to allow duplicate keys to be present in tracestate entries, modifying the `NewTraceState` constructor to rewrite the duplicate `es` keys according to the new W3C spec, but leaving 3rd party duplicates untouched. Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com> Co-authored-by: Marc Lopez Rubio <marc5.12@outlook.com>

axw · 2022-02-10T02:58:39Z

Verified using go.elastic.co/apm/v2 v2.0.0-20220209134054-640a916049c9 (current main), along with apmhttp/v2. To get apmhttp/v2 to work, I had to add a replace stanza:

replace go.elastic.co/apm/v2 v2.0.0 => go.elastic.co/apm/v2 v2.0.0-20220209134054-640a916049c9

I compared the v2 behaviour against the latest v1 agent.

package main

import (
        "fmt"
        "io"
        "io/ioutil"
        "net/http"
        "net/http/httptest"

        //"go.elastic.co/apm"
        //"go.elastic.co/apm/module/apmhttp"
        "go.elastic.co/apm/module/apmhttp/v2"
        "go.elastic.co/apm/v2"
)

func main() {
        srv := httptest.NewServer(apmhttp.Wrap(http.HandlerFunc(
                func(w http.ResponseWriter, r *http.Request) {
                        tx := apm.TransactionFromContext(r.Context())
                        fmt.Println(tx.TraceContext().State)
                },
        )))
        req, _ := http.NewRequest("GET", srv.URL, nil)
        req.Header.Set("Tracestate", "foo=bar,foo=bar")
        req.Header.Set("Traceparent", "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01")
        resp, _ := http.DefaultClient.Do(req)

        io.Copy(ioutil.Discard, resp.Body)
        resp.Body.Close()
}

With v1, tracestate is cleared out.
With v2, tracestate is maintained as-is.

github-actions bot added the agent-go label Jan 5, 2022

marclop commented Jan 5, 2022

View reviewed changes

marclop marked this pull request as ready for review January 5, 2022 08:48

marclop requested a review from a team January 5, 2022 08:48

stuartnelson3 approved these changes Jan 5, 2022

View reviewed changes

axw reviewed Jan 6, 2022

View reviewed changes

tracecontext.go Outdated Show resolved Hide resolved

Ignore keys from 3rd party systems

8465b5f

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

marclop requested review from axw and stuartnelson3 January 6, 2022 07:05

axw reviewed Jan 6, 2022

View reviewed changes

Simplify loop logic

9270b6a

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

axw approved these changes Jan 10, 2022

View reviewed changes

tracecontext.go Outdated Show resolved Hide resolved

tracecontext.go Outdated Show resolved Hide resolved

Fix fat finger typing

4344152

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>

marclop enabled auto-merge (squash) January 10, 2022 06:53

marclop merged commit 6a8e564 into elastic:master Jan 10, 2022

marclop deleted the b/fix-tracestate-duplicate-key-handling branch January 10, 2022 07:10

simitt added this to the 2.0 milestone Feb 9, 2022

simitt added the test-plan label Feb 9, 2022

axw self-assigned this Feb 10, 2022

axw added the test-plan-ok label Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracestate: Allow duplicate keys but rewrite them#1183

tracestate: Allow duplicate keys but rewrite them#1183
marclop merged 4 commits intoelastic:masterfrom
marclop:b/fix-tracestate-duplicate-key-handling

marclop commented Jan 5, 2022 •

edited

Loading

ghost commented Jan 5, 2022 •

edited by ghost

Loading

Build stats

Test stats 🧪

marclop Jan 5, 2022

stuartnelson3 Jan 5, 2022

Uh oh!

Uh oh!

stuartnelson3 left a comment

stuartnelson3 Jan 5, 2022

stuartnelson3 Jan 5, 2022

marclop Jan 6, 2022

stuartnelson3 Jan 5, 2022

marclop Jan 6, 2022

stuartnelson3 Jan 5, 2022

Uh oh!

Uh oh!

Uh oh!

axw Jan 6, 2022

axw Jan 6, 2022

marclop Jan 10, 2022

axw left a comment

Uh oh!

Uh oh!

axw commented Feb 10, 2022

Labels

4 participants

Conversation

marclop commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

ghost commented Jan 5, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartnelson3 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

axw commented Feb 10, 2022

Labels

4 participants

marclop commented Jan 5, 2022 •

edited

Loading

ghost commented Jan 5, 2022 •

edited by ghost

Loading