-
Notifications
You must be signed in to change notification settings - Fork 1
add ability to disable deadline propagation #42
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
Conversation
Generate changelog in
|
deadlineState.set(providedDeadline); | ||
} | ||
|
||
private static void checkExpiration(long deadline, boolean internal) { | ||
private static void checkExpiration(long deadline, boolean internal, boolean _disablePropagation) { |
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.
should we encode disablePropagation
as a separate tag on the meter?
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 think that would be helpful
* will result in a no-op assuming a deadline has previously be set for this trace (e.g. via a previous call to | ||
* {@link #parseFromRequest}). | ||
*/ | ||
public static void disableFurtherDeadlinePropagation() { |
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.
Should Optional<Duration> getRemainingDeadline()
return an empty optional after this is called?
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.
Outside of metrics, I think this should be equivalent to deadlineState.remove()
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. disableFurtherDeadlinePropagation
doesn't mean that the deadline no longer exists in the current TraceLocal state, just that we won't add the header on future calls to encodeToRequest
. maybe there is a valid use case for checking the deadline value even after we have disabled propagation?
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.
Outside of metrics, I think this should be equivalent to
deadlineState.remove()
i think that's perhaps fair, though it narrows some possibilities quite a bit.
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 updated the implementation to return Optional.empty()
in this case.
…n empty when propagation disabled
@@ -18,4 +18,12 @@ namespaces: | |||
docs: A deadline expiration was caused due to the inability to meet an | |||
externally provided deadline, such as a server being unable to | |||
complete required work before a client-provided deadline elapses. | |||
- name: propagationDisabled |
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.
perhaps something like action
or intent
with values along the lines of [propagate, ignore]
? I often find boolean metric tags a bit harder to understand, and they don't leave much opportunity to add additional states if we decide to roll out a feature flag later on
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.
ya, i'll rephrase. it's a little nuanced as i don't want to convey that the intent was to ignore enforcement of a deadline expiration originally, just that it should be ignored from a certain point onwards (and, the meter marked with this tag will only be marked after that point and so the intent is a little ambiguous).
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.
let me know what you think of this phrasing: https://github.com/palantir/deadlines-java/pull/42/files#diff-5415ea5dd248a8ccb374b1e951c3c3c0e320974aa75934eb6049080f95900b34R21-R33
Invalidated by push of 3de80d8
👍 |
Adds a method to disable further propagation of deadline values via the
Deadlines.encodeToRequest
method. The use case for this is to prevent RPC call stacks from continuing to sendExpect-Within: 0
after a deadline has expired, which can lead to quite a bit of noise in reported metrics and logging.Callers can use
Deadlines.disableFurtherDeadlinePropagation()
to instruct the library to ignore the internal deadline state on further calls toencodeToRequest
. Future calls toDeadlines.getRemainingDeadline()
will returnOptional.empty()
.Callers are expected to use this only when they can ensure that further operations should not be subject to deadline enforcement. A primary use case for this is for requests that spin off background work which may continue to make requests, even when the original request handler has terminated. In those instances, callers may wish to stop enforcing the deadline as it is quite possible a background task will make a request well into the future after the original deadline is expired, and the semantics of that are somewhat of a judgement call. Adding this flag will allow callers to control the fate of such future requests with respect to deadline enforcement.
This change adds the following:
Deadlines.disableFurtherDeadlinePropagation()
method, which alters the internal state of the current deadline within the TraceLocal, setting a flag to disable propagationintent
tag to the deadline expiration meter metric reported via this library; this tag will be set toignore
in scenarios whereencodeToRequest
was called afterdisableFurtherDeadlinePropagation
was called, andpropagate
otherwise==COMMIT_MSG==
add ability to disable deadline propagation
==COMMIT_MSG==