add functionality for --force-parent-span-id#241
Conversation
tobert
left a comment
tobert
left a comment
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.
| | --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
| "--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
| } | ||
| 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.
tobert
left a comment
tobert
left a comment
There was a problem hiding this comment.
My bad for thinking tests passed before. They don't. I think if you update the test case to be hex it'll work fine.
|
hey @tobert, thanks for the feedback. I pushed that change from string to hex, so hopefully it should pass now. 🤞 |
tobert
left a comment
tobert
left a comment
There was a problem hiding this comment.
I pushed a few more changes and a fix to get this ready to merge. Thanks!
2 participants
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