-
Notifications
You must be signed in to change notification settings - Fork 69
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
Deprecates Sender for much simpler BytesMessageSender #244
Merged
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# zipkin-reporter rationale | ||
|
||
## Sending an empty list is permitted | ||
|
||
Historically, we had a `Sender.check()` function for fail fast reasons, but it was rarely used and | ||
rarely implemented correctly. In some cases, people returned `OK` having no knowledge of if the | ||
health was good or not. In one case, Stackdriver, a seemingly good implementation was avoided for | ||
directly sending an empty list of spans, until `check()` was changed to do the same. Rather than | ||
define a poorly implementable `Sender.check()` which would likely still require sending an empty | ||
list, we decided to document a call to send no spans should pass through. | ||
|
||
Two known examples of using `check()` were in server modules that forward spans with zipkin reporter | ||
and finagle. `zipkin-finagle` is no longer maintained, so we'll focus on the server modules. | ||
|
||
zipkin-stackdriver (now zipkin-gcp) was both important to verify and difficult to implement a | ||
meaningful `check()`. First attempts looked good, but would pass even when users had no permission | ||
to write spans. For this reason, people ignored the check and did out-of-band sending zero spans to | ||
the POST endpoint. Later, this logic was made the primary impl of `check()`. | ||
|
||
In HTTP senders a `check()` would be invalid for non-intuitive reasons unless you also just posted | ||
no spans. For example, while zipkin has a `/health` endpoint, most clones do not implement that or | ||
put it at a different path. So, you can't check with `/health` and are left with either falsely | ||
returning `OK` or sending an empty list of spans. | ||
|
||
Note that zipkin server does obviate calls to storage when incoming lists are empty. This is not | ||
just for things like this, but 3rd party instrumentation which bugged out and sent no spans. | ||
|
||
Messaging senders came close to implementing health except would suffer similar problems as | ||
Stackdriver did. For example, verifying broker connectivity doesn't mean the queue or topic works. | ||
While you can dig around and solve this for some brokers, it ends up the same situation. | ||
|
||
Another way could be to catch an exception from a prior "POST", and if that failed, return a | ||
corresponding status. This could not be for fail-fast because the caller wouldn't have any spans to | ||
send, yet. It is complicated code for a function uncommon in instrumentation and the impl would be | ||
hard to reason with concurrently. | ||
|
||
The main problem is that we used the same `Component` type in reporter as we did for zipkin server, | ||
which defined `check()` in a hardly used and hardly implementable way except sending no spans. | ||
|
||
We had the following choices: | ||
|
||
* force implementation of `check()` knowing its problems and that it is usual in instrumentation | ||
* document that implementors can skip `send(empty)` even though call sites use this today | ||
* document that you should not skip `send(empty)`, so that the few callers can use it for fail-fast | ||
|
||
The main driving points were how niche this function is (not called by many, or on interval), | ||
and how much work it is to implement a `check()` vs allowing an empty send to proceed. In the | ||
current code base, the only work required for the latter was documentation, as all senders would | ||
pass an empty list. Secondary driving force was that the `BytesMessageSender` main goal is easier | ||
implementation and re-introducing a bad `check()` api gets in the way of this. | ||
|
||
Due to the complexity of this problem, we decided that rather to leave empty undefined, document | ||
sending empty is ok. This allows a couple users to implement a fail-fast in a portable way, without | ||
burdening implementers of `BytesMessageSender` with an unimplementable or wrong `check()` function | ||
for most platforms. |
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
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
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
Oops, something went wrong.
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.
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.
almost everything implemented this with a post to empty list, and in any case
check
was almost never called!