Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add w3 trace context support to cmd #220

Closed
wants to merge 1 commit into from

Conversation

cewkrupa
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

Adds a --w3 boolean flag to cmd.

If this flag is set, instead of outputting a marshalled beeline trace propagation context into the HONEYCOMB_TRACE environment variable, cmdwill output a [W3 trace context](https://docs.honeycomb.io/troubleshoot/product-lifecycle/recommended-migrations/migrate-from-beelines/go/#interoperability-with-opentelemetry) into the environment variablesHONEYCOMB_W3_TRACEPARENTandHONEYCOMB_W3_TRACESTATE`.

How to verify that this has the expected result

Great question! I don't know how to test this

@@ -47,6 +48,7 @@ will be launched via "bash -c" using "exec". The shell can be changed with the
name := strings.TrimSpace(args[2])
quiet, _ := cmd.Flags().GetBool("quiet")
shell, _ := cmd.Flags().GetString("shell")
useW3, _ := cmd.Flags().GetBool("w3")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally open to a flag name that makes more sense

_, headers := propagation.MarshalW3CTraceContext(context.Background(), prop)
cmd.Env = append(os.Environ(),
"HONEYCOMB_W3_TRACEPARENT="+headers[propagation.TraceparentHeader],
"HONEYCOMB_W3_TRACESTATE="+headers["tracestate"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason TraceparentHeader is exported, but TracestateHeader isn't

@cewkrupa
Copy link
Contributor Author

After discussing with @robbkidd -- even though this would generate W3C-compliant header values, these changes would not actually connect the build event traces to an OTel process on the other side of cmd. Closing this because it doesn't bring much value without also overhauling more of buildevent's internals to better support W3C/Otel.

@cewkrupa cewkrupa closed this Sep 18, 2024
@cewkrupa cewkrupa deleted the cewkrupa/add-w3-tracecontext-support-to-cmd branch September 18, 2024 16:47
@robbkidd
Copy link
Member

The discussion with @cewkrupa sparked an idea. I have a hypothesis—a wholly untested hypothesis—that a judicious application of bash could go some way towards buildevents trace and span IDs being W3C compliant.

With inspiration from the not-officially-OpenTelemetry otel-bash project's generation of traceid and spanid:

  • trace ID
    • generate a W3C trace ID at the very beginning of your CI workflow
    • persist that value consistently through all subsequent steps
    • make it available as an environment variable
    • use it in place of the CI provider's trace identifier in all buildevents invocations
  • span ID

With those two values consistently available, something running within a job step that needs a TRACEPARENT header can have one constructed from these two parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate 8-byte span IDs to conform with OpenTelemetry spec (instead of 16-byte currently)
2 participants