add functionality for --force-parent-span-id #241
Conversation
There was a problem hiding this comment.
PR overall is high quality and matches the code around it. Thank you!
The "parentdeadbeef" issue I noted in comments is a mistake I made in parseHex() or elsewhere that will need a follow-up. I'm inclined to merge this as-is, and follow up with another PR to fix the hex parsing issue.
README.md
Outdated
| @@ -119,6 +119,7 @@ then config file, then environment variables. | |||
| | --attrs | OTEL_CLI_ATTRIBUTES | span_attributes | k=v,a=b | | |||
| | --force-trace-id | OTEL_CLI_FORCE_TRACE_ID | force_trace_id | 00112233445566778899aabbccddeeff | | |||
| | --force-span-id | OTEL_CLI_FORCE_SPAN_ID | force_span_id | beefcafefacedead | | |||
| | --force-parent-span-id | OTEL_CLI_FORCE_PARENT_SPAN_ID | force_parent_span_id | parentbeefcafede | | |||
There was a problem hiding this comment.
minor nitpick, "parentbeefcafede" isn't hex
data_for_test.go
Outdated
| Config: FixtureConfig{ | ||
| CliArgs: []string{ | ||
| "status", | ||
| "--endpoint", "{{endpoint}}", | ||
| "--force-trace-id", "00112233445566778899aabbccddeeff", | ||
| "--force-span-id", "beefcafefacedead", | ||
| "--force-parent-span-id", "parentbeefcafede", |
There was a problem hiding this comment.
following off prior comment, if this passes, we have a test & safety gap allowing non-hex to pass through
| @@ -106,6 +106,10 @@ func NewProtobufSpanWithConfig(c Config) *tracepb.Span { | |||
| span.SpanId, err = parseHex(c.ForceSpanId, 8) | |||
| c.SoftFailIfErr(err) | |||
| } | |||
| if c.ForceParentSpanId != "" { | |||
| span.ParentSpanId, err = parseHex(c.ForceParentSpanId, 8) | |||
| c.SoftFailIfErr(err) | |||
There was a problem hiding this comment.
I wonder why this isn't failing?
I'm really glad you built the test this way, this should be failing and it's problematic if it's not.
|
hey @tobert, thanks for the feedback. I pushed that change from string to hex, so hopefully it should pass now. 🤞 |
similar to
--force-trace-idand--force-span-id, this PR adds the ability to specify--force-parent-span-idorOTEL_CLI_FORCE_PARENT_SPAN_IDto override a parent span id.cc/ #240