Use base64+json for trace context encoding #1775

goodspark · 2022-10-04T00:55:12Z

Back when I added trace propagation to the agent, I was short-sighted and used the golang-binary format. Since the trace context was meant to be propagated between builds of a pipeline, it was safe to assume the consumers would always be able to decode golang-binary formats.

But now I want to trace additional spans within non-Golang code and link it to the parent trace of the pipeline build. This will let me gather hollistic metrics about specific failure modes and performance of various commands/tooling running in my repos.

So I picked JSON, since it's a format all modern languages can easily parse without the need for a special library.

TODOs

Tests
Test a build and verify traces go through and trace context is properly passed to subprocesses

goodspark · 2022-10-05T01:57:31Z

tracetools/propagate.go

@@ -3,7 +3,7 @@ package tracetools
 import (


Test:

Launched an agent that was configured to use the Datadog tracing backend and setup a DD agent for it to talk to.
Ran a job that simply printed out the env vars:

[2022-10-05T01:52:48Z] BUILDKITE_TRACE_CONTEXT=eyJ4LWRhdGFkb2ctcGFyZW50LWlkIjoiNDQ4OTAzMDg0NTI3NTk3ODUxMSIsIngtZGF0YWRvZy1zYW1wbGluZy1wcmlvcml0eSI6IjIiLCJ4LWRhdGFkb2ctdGFncyI6Il9kZC5wLmRtPS00IiwieC1kYXRhZG9nLXRyYWNlLWlkIjoiODk0NzQ0MDA1Njc4MTEwNTcxNCJ9Cg==

Decoding it:

{"x-datadog-parent-id":"4489030845275978511","x-datadog-sampling-priority":"2","x-datadog-tags":"_dd.p.dm=-4","x-datadog-trace-id":"8947440056781105714"}

Then launched another job but with the above env vars given as inputs (to mimic distributed tracing).

[2022-10-05T02:05:04Z] BUILDKITE_TRACE_CONTEXT=eyJ4LWRhdGFkb2ctcGFyZW50LWlkIjoiODE0NzEzODA3MDc2MDA0OTEzMyIsIngtZGF0YWRvZy1zYW1wbGluZy1wcmlvcml0eSI6IjIiLCJ4LWRhdGFkb2ctdGFncyI6Il9kZC5wLmRtPS00IiwieC1kYXRhZG9nLXRyYWNlLWlkIjoiODk0NzQ0MDA1Njc4MTEwNTcxNCJ9Cg==

Decoded:

{"x-datadog-parent-id":"8147138070760049133","x-datadog-sampling-priority":"2","x-datadog-tags":"_dd.p.dm=-4","x-datadog-trace-id":"8947440056781105714"}

Verified traces still get sent and that the two builds were linked together in Datadog.

goodspark · 2022-10-05T02:14:09Z

tracetools/propagate.go

@@ -22,8 +22,7 @@ func EncodeTraceContext(span opentracing.Span, env map[string]string) error {
 	}

 	buf := bytes.NewBuffer([]byte{})
-	enc := gob.NewEncoder(buf)
-	if err := enc.Encode(textmap); err != nil {
+	if err := json.NewEncoder(buf).Encode(textmap); err != nil {


Switching immediately means there will be a period when people might lose trace linking (but not traces) while they roll out the new agent across their CI fleet. Seemed simpler than trying to plumb through all the env vars and configs, which was what I started with but was having trouble getting working.

so i'm not an opentracing expert, but the value of the BUILDKITE_TRACE_CONTEXT env var is something that downstream (ie, non-buildkite agent) processes have to decode this to get a trace context, right? it's not just an opaque value?

if this is the case, would the fact that we're switching encodings here means that tracing setups that previously worked would have to change their decoding strategy? if so, this would be a breaking change, and i don't think we'd be able to ship it.

there are other options, i think - we could expose a second env var like BUILDKITE_TRACE_CONTEXT_BASE64 or something. We could also do the other thing and wire up all the CLI flags and configs and such, though i agree with you that that's not a super great solution; not least of which because it's a pain to implement, but it's also good to keep the number of options on the agent down where we can.

I wound up adding an arg and plumbing it through. Turns out the code I had before was mostly fine, I just had typos that was breaking some agent-to-bootstrap plumbing.

goodspark · 2022-10-08T04:58:49Z

tracetools/propagate.go

@@ -3,7 +3,7 @@ package tracetools
 import (


It'll also be interesting to see if we can use the proposed env vars in open-telemetry/opentelemetry-specification#740 (which is also mentioned in #1663) as a move towards more standardization. This will help others reusing libraries native to whatever language/tool they're using while still maintaining trace linking across process boundaries.

moskyb

the code of this looks great, but i have some concerns about the backward-compatibility of it. Provided we can sort those out, i'd love to get this into the agent - more, more normal encoding methods is a good thing!

moskyb · 2022-10-11T02:51:59Z

tracetools/propagate.go

@@ -22,8 +22,7 @@ func EncodeTraceContext(span opentracing.Span, env map[string]string) error {
 	}

 	buf := bytes.NewBuffer([]byte{})
-	enc := gob.NewEncoder(buf)
-	if err := enc.Encode(textmap); err != nil {
+	if err := json.NewEncoder(buf).Encode(textmap); err != nil {


so i'm not an opentracing expert, but the value of the BUILDKITE_TRACE_CONTEXT env var is something that downstream (ie, non-buildkite agent) processes have to decode this to get a trace context, right? it's not just an opaque value?

if this is the case, would the fact that we're switching encodings here means that tracing setups that previously worked would have to change their decoding strategy? if so, this would be a breaking change, and i don't think we'd be able to ship it.

there are other options, i think - we could expose a second env var like BUILDKITE_TRACE_CONTEXT_BASE64 or something. We could also do the other thing and wire up all the CLI flags and configs and such, though i agree with you that that's not a super great solution; not least of which because it's a pain to implement, but it's also good to keep the number of options on the agent down where we can.

agates4 · 2022-11-07T15:52:50Z

I'm excited about this PR - it would help us send additional traces through to Datadog!

goodspark · 2023-02-10T17:53:45Z

This has been sitting around for months. Any chance someone from Buildkite can review this?

Back when I added trace propagation to the agent, I was short-sighted and used the golang-binary format. Since the trace context was meant to be propagated between builds of a pipeline, it was safe to assume the consumers would always be able to decode golang-binary formats. But now I want to trace additional spans within code that runs within a Buildkite pipeline and link it to the parent trace of the pipeline build. This will let me gather hollistic metrics about specific failure modes and performance of various commands/tooling running in my repos. So JSON was added, since it's a format all modern languages can easily parse without the need for a special library.

goodspark force-pushed the json-trace-context branch 2 times, most recently from fd54544 to 4d53ce9 Compare October 5, 2022 01:55

goodspark commented Oct 5, 2022

View reviewed changes

goodspark changed the title ~~Provide option for base64 json trace context encoding~~ USe base64+json for trace context encoding Oct 5, 2022

goodspark marked this pull request as ready for review October 5, 2022 02:15

goodspark changed the title ~~USe base64+json for trace context encoding~~ Use base64+json for trace context encoding Oct 5, 2022

goodspark commented Oct 8, 2022

View reviewed changes

moskyb reviewed Oct 11, 2022

View reviewed changes

goodspark force-pushed the json-trace-context branch from 4d53ce9 to d2ccefa Compare October 12, 2022 04:16

goodspark requested a review from moskyb October 12, 2022 04:19

goodspark force-pushed the json-trace-context branch from ac82c94 to 45a5478 Compare November 22, 2022 05:54

goodspark force-pushed the json-trace-context branch from 45a5478 to 52e65fc Compare December 8, 2022 06:24

goodspark force-pushed the json-trace-context branch from 52e65fc to a823ce7 Compare January 5, 2023 23:50

goodspark force-pushed the json-trace-context branch from a823ce7 to 14e6359 Compare January 24, 2023 04:20

goodspark added 3 commits March 18, 2023 19:09

Fix

2db5418

Consistent env var and arg names

4a4d877

goodspark force-pushed the json-trace-context branch from 14e6359 to 4a4d877 Compare March 18, 2023 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use base64+json for trace context encoding #1775

Use base64+json for trace context encoding #1775

goodspark commented Oct 4, 2022 •

edited

goodspark Oct 5, 2022

goodspark Oct 5, 2022

moskyb Oct 11, 2022

goodspark Oct 12, 2022

goodspark Oct 8, 2022

moskyb left a comment

moskyb Oct 11, 2022

agates4 commented Nov 7, 2022

goodspark commented Feb 10, 2023

Use base64+json for trace context encoding #1775

Are you sure you want to change the base?

Use base64+json for trace context encoding #1775

Conversation

goodspark commented Oct 4, 2022 • edited

goodspark Oct 5, 2022

Choose a reason for hiding this comment

goodspark Oct 5, 2022

Choose a reason for hiding this comment

moskyb Oct 11, 2022

Choose a reason for hiding this comment

goodspark Oct 12, 2022

Choose a reason for hiding this comment

goodspark Oct 8, 2022

Choose a reason for hiding this comment

moskyb left a comment

Choose a reason for hiding this comment

moskyb Oct 11, 2022

Choose a reason for hiding this comment

agates4 commented Nov 7, 2022

goodspark commented Feb 10, 2023

goodspark commented Oct 4, 2022 •

edited