-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add beeline-style propagation of the cmd context to called commands #74
Conversation
e89a213
to
f3a35b0
Compare
This is my first significant piece of go, so caveat emptor 😅 please be gentle and thorough. I'm not convinced that my choice of environment variable is great and would be open to better suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally, the PR looks good. I've one super minor suggestion about reusing a variable.
I don't think HTTP_X_HONEYCOMB_TRACE
is the right choice for an env var to hold trace state. I'll see if I can find a more suitable alternative. If this is something we are expecting to support, we'd need have consistency across the beelines to try to read the env var.
err := runCommand(subcmd) | ||
|
||
// copy out the current set of fields to avoid later modification | ||
localFields := map[string]interface{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: create a variable for the spanID so we don't need to fmt.Sprintf twice 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. fixed that.
cmd_cmd.go
Outdated
fmt.Println("running /bin/bash -c", subcmd) | ||
cmd := exec.Command("/bin/bash", "-c", subcmd) | ||
|
||
cmd.Env = append(os.Environ(), | ||
"HTTP_X_HONEYCOMB_TRACE=" + propagation.MarshalHoneycombTraceContext(prop), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is right thing to use either, but then also I don't have an alternative suggestion.
This env var is only used in beeline-ruby: https://github.com/search?q=org%3Ahoneycombio+HTTP_X_HONEYCOMB_TRACE&type=code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this to only HONEYCOMB_TRACE
. This matches the other HONEYCOMB_*
environment variables as well as the X-Honeycomb-Trace
HTTP header.
I should also add a bit of docs around that, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth noting that the HTTP_X_HONEYCOMB_TRACE
is not an environment variable in the ruby beeline, it's only pulled from the rack environment
5467f4a
to
50b0a2c
Compare
Also added a bit of documentation into the README on how to use the variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this is good. I don't believe other beelines try to read an env var for trace context and HONEYCOMB_TRACE
is a good choice. However, given a string value most beelines can construct a context object. It's a little manual for now and we can think about how we'd like to handle that more elegantly later.
I'd like a second review from @robbkidd for the ruby example, as it's not my primary language.
README.md
Outdated
# establish a command-level span, linking to the buildevent | ||
process_span = Honeycomb.start_span(name: File.basename($PROGRAM_NAME), serialized_trace: ENV['HONEYCOMB_TRACE']) | ||
Honeycomb.add_field_to_trace('process.full_name', $PROGRAM_NAME) | ||
Honeycomb.add_field_to_trace('process.args', ARGV) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example, but I think we should be careful advocating adding cmd args as trace fields. Many apps provide sensitive data via command args which would be be leaked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove that line, but fwiw, buildevents itself captures ARGV.
The example is straight from https://github.com/puppetlabs/rspec_honeycomb_formatter/blob/e32d0a6d5115a3175cbee7506191b8a8578dfb59/lib/rspec_honeycomb_formatter.rb#L14-L29 and quite simliar to the other place I've used this: https://github.com/puppetlabs/puppet_litmus/blob/ed88fa024bffe50008ddf3d0756d7a9603fbdcb7/lib/puppet_litmus/rake_helper.rb#L12 |
README.md
Outdated
Honeycomb.add_field_to_trace('process.args', ARGV) | ||
|
||
# override the HONEYCOMB_TRACE for sub-processes | ||
ENV['HONEYCOMB_TRACE'] = process_span.to_trace_header unless ENV['HONEYCOMB_TRACE'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of this line is that ENV['HONEYCOMB_TRACE']
will only be set to the new trace header if the environment variable is not already set, the equivalent of ...
ENV['HONEYCOMB_TRACE'] ||= process_span.to_trace_header
The mention of "override" in the comment has me thinking the expected behavior will be to set the starting trace context for child processes to the trace information modified above in the parent process. But I don't think that is what is happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oof, you're right! And I was worried about my golang skills 😅
When using something like https://github.com/puppetlabs/rspec_honeycomb_formatter , it would be nice to connect the individual spans to the bigger picture of the buildevent steps and commands. This change uses the beeline's `PropagationContext` marshaling to pass on the current event to the called command. In the style of the rest of the HONEYCOMB_* environment variables and the similarily named `X-Honeycomb-Trace` HTTP header, this establishes `HONEYCOMB_TRACE` to pass the propagation context to other processes.
50b0a2c
to
7c0838d
Compare
Updated the ruby example to fix the ENV override as noticed by @robbkidd and commented/disabled the ARGV capturing as flagged by @MikeGoldsmith |
Thanks to @robbkidd for pointing out my folly in honeycombio/buildevents#74 (comment)
This is to align this with the conversation and changes in honeycombio/buildevents#74
This is to align this with the conversation and changes in honeycombio/buildevents#74
Hand-build binary from honeycombio/buildevents#74
Hand-build binary from honeycombio/buildevents#74
Hand-build binary from honeycombio/buildevents#74
Hand-build binary from honeycombio/buildevents#74
Hand-build binary from honeycombio/buildevents#74
Hand-build binary from honeycombio/buildevents#74
I've tested a locally built buildevents binary in puppetlabs/puppetlabs-testing#324 and got a complete trace from GHA through rake, through the process rake starts, through the webservice call to our provisioning backend to the call to terraform applying and including our rspec tests running in that github action and calling into the newly provisioned machine: pretty chuffed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @DavidS - thanks for the extra work on this. I think looks really good and I'm happy with the generic env var name that is used to pass trace context onto sub-processes to parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ite_env * 'main' of github.com:honeycombio/buildevents: (40 commits) add apply labels workflow (#93) GitLab CI example (#90) Build an arm64 release (#91) * publish a draft release to GitHub (#87) Prepare for 0.5.0 release, add changelog (#86) Add support for overriding default fields (#76) fix providerAzurePipelines in main.go (#84) add bitbucket support (#85) Ensure PRs from forks can run CI checks (#82) added quiet option to cmd (#80) Use public CircleCI context for build secrets (#77) add codeowners with integration-team (#78) Encode spaces in dataset names for the URL (#73) Add beeline-style propagation of the cmd context to called commands (#74) we're on circle for this, no need to build on travis any longer (#69) make watch job more efficient (#70) drop vendor/ and upgrade go to 1.15.2 (#68) add GitHub actions as a supported CI provider, with data from env vars Correct a download link Add support for Azure Piplines and some initial env vars ... # Conflicts: # README.md # cmd_root.go # common.go # main.go
When using something like https://github.com/puppetlabs/rspec_honeycomb_formatter , it would be nice to connect the individual spans to the bigger picture of the buildevent steps and commands. This change uses the beeline's
PropagationContext
marshaling to pass on the current event to the called command.In the style of the rest of the HONEYCOMB_* environment variables and the similarily named
X-Honeycomb-Trace
HTTP header, this establishesHONEYCOMB_TRACE
to pass the propagation context to other processes.I'm also adopting this naming for the two other projects my team is maintaining where trace contexts are passed around: