add span background wait #73
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 4 commits
September 8, 2021 13:08
Before e.g. `--timeout 60` would fail to parse as a duration but it seems sensible to me to default to seconds when nothing else is specified.
Signed-off-by: Amy Tobey <atobey@equinix.com>
Signed-off-by: Amy Tobey <atobey@equinix.com>
This started with #67. I tried a bunch of variations of re-execing otel-cli and did manage it get it to detach, but could not preserve parent pid for the os.Getppid monitoring trick. This would mean having to watch the process table or some other undesirable and difficult to port hack so I ended up here. Since the code to create a client connection already blocks a bit looking for the socket to start up, all this does is that. It starts the json-rpc client, which connects then runs a no-op RPC, then returns. At that point we know the original span background is fully alive and ready to go so there's no more race conditions. Where I ran into this was this use case: ```sh local otelcarrier=$(mktemp) local sockdir=$(mktemp -d) otel-cli span background \ --name "$0" \ --service "osie" \ --sockdir "$sockdir" --tp-carrier "$otelcarrier" \ --timeout 1800 & otel-cli span background --wait --sockdir "$sockdir" --timeout 10 source "$otelcarrier" # load the new traceparent ``` Signed-off-by: Amy Tobey <atobey@equinix.com>
tobert
pushed a commit
to tobert/osie
that referenced
this pull request
Sep 9, 2021
I forgot I didn't have --wait merged into v0.0.15 so I went off and got that polished off in these PRs: equinix-labs/otel-cli#73 equinix-labs/otel-cli#74 Signed-off-by: Amy Tobey <atobey@equinix.com>
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm working on a use case where I want this:
It seems nice to source the carrier file so everything that happens in the script is a child to the background span. This has a race condition though, otel-cli might not be started up all the way and may not have written out the tp-carrier file yet. I tried to figure out a way to make otel-cli background itself but Go doesn't provide a way to do that and keep the ppid so this was the next best option.
--tp-carrier only writes the bare traceparent string to the file, I have another PR after this that changes that to be the same as the --tp-print output and respects --tp-export so it's sourceable as expected.
This adds --wait to `otel-cli span background so you can avoid the race:
Also fixes e.g.
--timeout 60so now otel-cli assumes you want seconds for bare numbers. You can still say--timeout 60sand--timeout 500ms.Also go mod tidy I missed when I added
statusand removed the old test mode.