Skip to content

Add Failure to query result#503

Merged
bergundy merged 1 commit intotemporalio:masterfrom
bergundy:query-failure
Dec 6, 2024
Merged

Add Failure to query result#503
bergundy merged 1 commit intotemporalio:masterfrom
bergundy:query-failure

Conversation

@bergundy
Copy link
Member

@bergundy bergundy commented Dec 5, 2024

What changed?

Add a Failure field to:

  • QueryFailedFailure
  • WorkflowQueryResult
  • RespondQueryTaskCompletedRequest

Why?

Support encoding failure attributes for E2E encryption of messages and stack traces.

Server PR

Coming soon but I've already validated this change locally.

@bergundy bergundy requested review from a team as code owners December 5, 2024 23:54
Comment on lines +923 to +924
// The error message will be duplicated if the `failure` field is present to support callers that pre-date the
// addition of that field.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The error message will be duplicated if the `failure` field is present to support callers that pre-date the
// addition of that field.
// The error message may be totally or partially redundant with the `failure` field, if present, to support callers that pre-date the
// addition of that field.

I can imagine that, if this feature is supported, SDK might allow for a sensitive-information-free message separate from the full details.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, in which case the failure message will be the same as error_message (e.g. "Encoded...") and the sensitive information will be in the encoded details.

Copy link
Contributor

@cretz cretz Dec 6, 2024

Choose a reason for hiding this comment

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

👍 For SDK implementers worker side, error_message is expected to always be set to failure.message. And for SDK implementers client side, error_message should be ignored if failure is non-null. But yes, both error_message and failure.message will have "Encoded failure" for encoded failures (and client side since error_message is ignored when failure is present, the failure should be decoded and the decoded failure message would be used).

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is that it might make sense to allow setting error_message to something different, if we want to allow for specifying some un-encoded message.

But, regardless, if the expectation is that, any time failure is used, the message is always just "Encoded failure" or w/e, then we should specify that in the docstrings too.

@bergundy bergundy merged commit 9e71963 into temporalio:master Dec 6, 2024
@bergundy bergundy deleted the query-failure branch December 6, 2024 17:22
bergundy added a commit to temporalio/temporal that referenced this pull request Dec 19, 2024
## What changed?

Attach a Failure object from query failures to the QueryFailure
serviceerror.

## Why?

Allows encryption of failure messages and stack traces.

See also temporalio/api/pull/503.
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.

4 participants