question/improve: add helper functions for changing options

[SeJIya]

Override default options (ex. activity options) need using .WithActivityOptions method because options included in context will be rewritten with define in proto (context options ignoring if have default - https://github.com/cludden/protoc-gen-go-temporal/blob/main/gen/example/v1/example_temporal.pb.go#L857) . .WithActivityOptions - method overide all options, not have varinat chnaging one.

Before this codegen we using options in ctx for execute activities/child workflow as:

// w.baseActivityOptions = workflow.ActivityOptions{
// 	HeartbeatTimeout:    cfg.TemporalConfig.ActivityHeartbeatTimeout, // configurate from ENV/config
// 	WaitForCancellation: true,
// },
func (w *WorkflowV1) Example(ctx workflow.Context) (werr error) {
    activityCtx := workflow.WithActivityOptions(ctx, w.baseActivityOptions)
    
    deleteCtx := workflow.WithStartToCloseTimeout(activityCtx, 10 * time.Secund)
    // execute activity 
    publishCtx := workflow.WithStartToCloseTimeout(activityCtx, 15 * time.Secund)
    publishCtx = workflow.WithRetryPolicy(publishCtx, temporal.RetryPolicy{
	 MaximumAttempts: 3
    })
    // execute activity 

for now if based from generated code and need change one default options already defined in proto:

func (w *WorkflowV1) Example(ctx workflow.Context) (werr error) {
    ao := w.baseActivityOptions
    ao.StartToCloseTimeout = 10 * time.Secund
    opts = examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao)
    examplev1.NewExampleV1(ctx, *examplev1.ExampleV1Request{}, opts)

My idea/concept for adding all methods for options activity & workflow (https://github.com/temporalio/sdk-go/blob/master/workflow/activity_options.go#L59 - for activity example):

func (w *WorkflowV1) Example(ctx workflow.Context) (werr error) {
	opts = examplev1.NewExampleV1ActivityOptions().WithActivityOptions(w.baseActivityOptions).WithStartToCloseTimeout(10 * time.Second)
	// or
	opts = examplev1.NewExampleV1ActivityOptions().WithStartToCloseTimeout(10 * time.Second)
	examplev1.NewExampleV1(ctx, *examplev1.ExampleV1Request{}, opts)

generated code after:

// NewExampleV1ActivityOptions sets default ActivityOptions
func NewExampleV1ActivityOptions() *ExampleV1ActivityOptions{
	return &ExampleV1ActivityOptions{
+		opts: &workflow.ActivityOptions{},
	}
}

// WithActivityOptions sets default ActivityOptions
func (opts *ExampleV1ActivityOptions) WithActivityOptions(options workflow.ActivityOptions) *DeleteFileV1ActivityOptions {
	opts.opts = &options
	return opts
}

+func (opts *ExampleV1ActivityOptions) WithStartToCloseTimeout(d time.Duration) *DeleteFileV1ActivityOptions {
+	opts.opts.StartToCloseTimeout = d
+	return opts
+}

// and more methods...

I need your opinion for this concept.

[cludden]

I like this proposal! That said, I think they should be eventually consistent. Consider the following example:

service Example {
  rpc Foo(FooInput) returns (FooOutput) {
    option (temporal.v1.activity) = {
      start_to_close_timeout: { seconds: 60 }
    }
  }
}

baseActivityOptions := workflow.ActivityOptions{StartToCloseTimeout: 30 * time.Second}
optsA := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(baseActivityOptions).WithStartToCloseTimeout(10 * time.Second)
optsB := examplev1.NewExampleV1ActivityOptions().WithStartToCloseTimeout(10 * time.Second).WithActivityOptions(baseActivityOptions)

calling either of the following should result in the same value applied to the activity's StartToCloseTimeout

examplev1.Foo(ctx, &examplev1.FooInput{}, optsA)
examplev1.Foo(ctx, &examplev1.FooInput{}, optsB)

This would require defining (and documenting) a preference order, e.g.

Explicit Option Call (e.g. WithStartToCloseTimeout) > WithActivityOptions > Schema Default

I think this is doable, just a little bit trickier than the sample after code above. I also think that the other options flavors (e.g. WorkflowOptions, ChildWorkflowOptions, LocalActivityOptions, etc) should probably do the same for consistency.

If we were to go in this direction, it's probably worth refactoring how the generated options code works. Currently the various resource method/functions build the necessary sdk options from the generated option structs (if provided) and handle applying defaults. I'd probably refactor the generated options to handle this part by adopting the builder pattern, i.e.:

type FooActivityOptions struct {
    opts                *workflow.ActivityOptions
    startToCloseTimeout time.Duration
    // ...
}

func (opts *FooActivityOptions) WithActivityOptions(ao workflow.ActivityOptions) {
    opts.opts = &ao
}

func (opts *FooActivityOptions) WithStartToCloseTimeout(d time.Duration) {
    opts.startToCloseTimeout = d
}

func (opts *FooActivityOptions) buildActivityOptions() (ao workflow.ActivityOptions, err error) {
    if opts.opts != nil {
        ao = *opts.opts
    }
    if d := opts.startToCloseTimeout; d > 0 {
        ao.StartToCloseTimeout = d
    }
    // if schema defines a default
    if ao.StartToCloseTimeout == 0 {
        ao.StartToCloseTimeout = 60000000000 // 1m
    }
    // ...
    return ao, nil
}

Thoughts?

[SeJIya]

• Current behavior for WithActivityOptions in ctx method in temporal SDK - "overwritting by the passed in value whole" - https://github.com/temporalio/sdk-go/blob/master/workflow/activity_options.go#L41

  this code return different result depending on the order

  ctxA := workflow.WithActivityOptions(ctx, workflow.ActivityOptions{
      HeartbeatTimeout: 100 * time.Second,
  })
  ctxA = workflow.WithHeartbeatTimeout(ctxA, 1337 * time.Second)
  // workflow.GetActivityOptions(ctxA).HeartbeatTimeout - 1337s
  
  ctxB := workflow.WithHeartbeatTimeout(ctx, 1337 * time.Second)
  ctxB = workflow.WithActivityOptions(ctxB, workflow.ActivityOptions{
      HeartbeatTimeout: 100 * time.Second,
  })
  // workflow.GetActivityOptions(ctxB).HeartbeatTimeout - 100s,

  we can:

  • follow default SDK behavior
  • use behavior with merging preference order

  I not against any option, choice for maintainer :)

• I also think that the other options flavors (e.g. WorkflowOptions, ChildWorkflowOptions, LocalActivityOptions, etc) should probably do the same for consistency.

  Agree, example in top with activity only for understanding concept

• If choosing behavior with merging preference order meaby not create field in struct and just use one more SDK options struct, and change it (reduce boilerplate code, depend SDK structs fields):

  type ExampleV1ActivityOptions struct {
      opts *workflow.ActivityOptions
      taskQueue string
      scheduleToCloseTimeout time.Duration
      // more nore fileds... (workflow options have many fields)
  }
  
  // to ->
  
  type ExampleV1ActivityOptions struct {
      opts *workflow.ActivityOptions // for WithActivityOptions
      co *workflow.ActivityOptions // for any custom options | co - custom options, i can't think of a better name its example only
  }

  then functions:

  func (opts *ExampleV1ActivityOptions) WithActivityOptions(options workflow.ActivityOptions) *DeleteFileV1ActivityOptions {
      opts.opts = &options
      return opts
  }
  
  func (opts *ExampleV1ActivityOptions) WithStartToCloseTimeout(d time.Duration) *DeleteFileV1ActivityOptions {
      opts.co.StartToCloseTimeout = d
      return opts
  }
  
  // ...
  
  func (opts *ExampleV1ActivityOptions) buildActivityOptions() (ao workflow.ActivityOptions, err error) {
      if opts.opts != nil {
          ao = *opts.opts
      }
      if d := opts.co.StartToCloseTimeout; d > 0 {
          ao.StartToCloseTimeout = d
      }
      // if schema defines a default
      if ao.StartToCloseTimeout == 0 {
          ao.StartToCloseTimeout = 60000000000 // 1m
      }
      // ...
      return ao, nil
  }

[cludden]

re:

we can:

• follow default SDK behavior
• use behavior with merging preference order

The main difference between this scenario and SDK's behavior, which is not documented well at the moment, is that the generated code starts with an empty options value. It then checks if the invocation provided an options value, and if so, replaces its initial options with those that were passed in by the With<X>Options builder option. It then applies schema defaults one-by-one to this value.

I think my preference would be to follow a similar approach with field-specific options, where these would take precedence over the user-provided options value and also the schema defaults.

1. empty options
2. replace with `With<X>Options` value, if specified
3. override with field-specific option value (e.g. `WithStartToCloseTimeout`), if specified
4. if the value is still empty and the schema defines a default value, use it

Using my previous example:

service Example {
  rpc Foo(FooInput) returns (FooOutput) {
    option (temporal.v1.activity) = {
      start_to_close_timeout: { seconds: 60 }
    }
  }
}

optsA := examplev1.NewExampleV1ActivityOptions()
// workflow.GetActivityOptions(ctx).StartToCloseTimeout -> 60s

ao := workflow.ActivityOptions{HeartbeatTimeout: 30 * time.Second}
optsB := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao)
// workflow.GetActivityOptions(ctx).StartToCloseTimeout -> 60s

ao.StartTocloseTimeout: 30 * time.Second}
optsC := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao)
// workflow.GetActivityOptions(ctx).StartToCloseTimeout -> 30s

optsD := examplev1.NewExampleV1ActivityOptions().WithStartToCloseTimeout(10 * time.Second)
// workflow.GetActivityOptions(ctx).StartToCloseTimeout -> 10s

optsE := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao).WithStartToCloseTimeout(10 * time.Second)
// workflow.GetActivityOptions(ctx).StartToCloseTimeout -> 10s

optsF := examplev1.NewExampleV1ActivityOptions().WithStartToCloseTimeout(10 * time.Second).WithActivityOptions(ao)
// workflow.GetActivityOptions(ctx).StartToCloseTimeout -> 10s

re:

If choosing behavior with merging preference order meaby not create field in struct and just use one more SDK options struct, and change it (reduce boilerplate code, depend SDK structs fields):

While I like this idea, and can appreciate the desire to minimize the size of the generated code, I'm not sure this would work for every field. Consider the following example:

service Example {
  rpc Foo(FooInput) returns (FooOutput) {
    option (temporal.v1.activity) = {
      wait_for_cancellation: true
    }
  }
}

optsA := examplev1.NewExampleV1ActivityOptions()
// workflow.GetActivityOptions(ctx).WaitForCancellation -> true

ao := workflow.ActivityOptions{WaitForCancellation: false}
optsB := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao)
// workflow.GetActivityOptions(ctx).WaitForCancellation -> true

optsB := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao).WithWaitForCancellation(false)
// workflow.GetActivityOptions(ctx).WaitForCancellation -> false

It's not possible to differentiate between an unspecified options value and an explicit false value. The generated code could handle this with some flavor of:

import (
    "go.temporal.io/sdk/workflow"
    "google.golang.org/protobuf/types/known/wrapperspb"
)

type FooActivityOptions struct {
    opts *workflow.ActivityOptions
    waitForCancellation *bool
    waitForCancellation *wrapperspb.BoolValue
}

I think a better opportunity for reduce boilerplate would be to refactor the generated code to use a single implementation of each option type instead of separate one for each rpc. When this was originally create prior to Go generics, per-rpc implementations were necessary to handle the WithLocal LocalActivityOption, but now I think this could either be implemented with generics or reflection.

[SeJIya]

I agree for this solution (added filds for structs) because nullable value without ptr for boolean not possible checking setted it or not (maybe other types same).

However, problem for boolean types (for example) in WithActivityOptions and default value in .proto not resolve without documentation how works it.

service Example {
  rpc Foo(FooInput) returns (FooOutput) {
    option (temporal.v1.activity) = {
      wait_for_cancellation: true
    }
  }
}

ao := workflow.ActivityOptions{WaitForCancellation: false, HeartbeatTimeout: 10 * time.Second}
opts := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao)
// execute activity options: `WaitForCancellation` -> `true`

// only this pattern working
ao := workflow.ActivityOptions{HeartbeatTimeout: 10 * time.Second}
opts := examplev1.NewExampleV1ActivityOptions().WithActivityOptions(ao).WithWaitForCancellation(false) // or with changing order `With` funcs calls
// execute activity options: `WaitForCancellation` -> `false`

[cludden]

Absolutely, currently it's not possible to override a schema default of true for a boolean option field to false because we're not able to differentiate between an unset value and an explicit false value, so a non-empty schema default (e.g. true) always takes precedence.

It would be possible if we added these proposed field-specific options. As part of this effort, updating the documentation to highlight the order of option precedence and behavior with empty vs. specific "falsey" values will need to be added.

[SeJIya]

I see u create PR, however i continue mind and detect case:

• use custom DataConverter for workflow

For ctx have func WithDataConverter() and this func not set filds in ChildWorkflowOptions - this struct not have field for it

Question: can we assume that DataConverter - option, and implement method WithDataConverter fot exlude using ctx func at all ?

example:

ctx = ctx.WithDataConverter(ctx, converter)
opts = examplev1.NewExampleV1WorkflowOptions().WithChildOptions(options)
examplev1.NewExampleV1(ctx, &examplev1.ExampleV1Request{}, opts)

->

if we assume that DataConverter - option:

opts = examplev1.NewExampleV1WorkflowOptions().WithChildOptions(options).WithDataConverter(conveter)
examplev1.NewExampleV1(ctx, &examplev1.ExampleV1Request{}, opts)

// in generated code:
type ExampleV1ChildOptions struct {
	opts                     *workflow.ChildWorkflowOptions
	// ...
	dataConverter            *converter.DataConverter
}

func ExampleV1Child(ctx workflow.Context, req *ExampleV1Request, options ...*ExampleV1ChildOptions) (*ExampleV1ChildRun, error) {
	opts := &workflow.ChildWorkflowOptions{}
	if len(options) > 0 && options[0].opts != nil {
		opts = options[0].opts
	}
	if opts.TaskQueue == "" {
		opts.TaskQueue = XnsTaskQueue
	}
	// ...
	ctx = workflow.WithChildOptions(ctx, *opts)
	if c := options[0].dataConverter; c != nil {
		ctx = workflow.WithDataConverter(ctx, *c)
	} 
        // ...
}

[cludden]

I like this proposal 👍 let's split it out as a separate issue/request.