Should we pass go context.Context to all the components?

[skyao]

In golang, context.Context is widely used and it's always be the first argument of go method and passed everywhere.

In dapr current implementation, for example, pkg/grpc/api.go, all the public method called by gRPC have the context object, and some them (service incation, actor ) pass context in the next step (always calling the component):

func (a *api) CallLocal(ctx context.Context, in *internalv1pb.InternalInvokeRequest) (*internalv1pb.InternalInvokeResponse, error) {
	callAllowed, errMsg := a.applyAccessControlPolicies(ctx, operation, httpVerb, a.appProtocol)
	resp, err := a.appChannel.InvokeMethod(ctx, req)
}

func (a *api) CallActor(ctx context.Context, in *internalv1pb.InternalInvokeRequest) (*internalv1pb.InternalInvokeResponse, error) {
	resp, err := a.actor.Call(ctx, req)
}
func (a *api) InvokeService(ctx context.Context, in *runtimev1pb.InvokeServiceRequest) (*commonv1pb.InvokeResponse, error) {
	resp, err := a.directMessaging.Invoke(ctx, in.Id, req)
}
func (a *api) GetActorState(ctx context.Context, in *runtimev1pb.GetActorStateRequest) (*runtimev1pb.GetActorStateResponse, error) {
	resp, err := a.actor.GetState(ctx, &req)
}

And some of them (pub-sub / binding / state ) don't pass context:

func (a *api) PublishEvent(ctx context.Context, in *runtimev1pb.PublishEventRequest) (*emptypb.Empty, error) {
	err = a.pubsubAdapter.Publish(&req)
}
func (a *api) SubscribeEvent(srv runtimev1pb.Dapr_SubscribeEventServer) error {
	err := curPubsub.Subscribe(req, func(msg *pubsub.NewMessage) error 
}
func (a *api) InvokeBinding(ctx context.Context, in *runtimev1pb.InvokeBindingRequest) (*runtimev1pb.InvokeBindingResponse, error) {
	resp, err := a.sendToOutputBindingFn(in.Name, req)
}
func (a *api) GetState(ctx context.Context, in *runtimev1pb.GetStateRequest) (*runtimev1pb.GetStateResponse, error) {
	getResponse, err := store.Get(&req)
}

So, there are big difference between components.

For example, actor api, all the method start with context argument :

type Actors interface {
	Call(ctx context.Context, req *invokev1.InvokeMethodRequest) (*invokev1.InvokeMethodResponse, error)
	Init() error
	Stop()
	GetState(ctx context.Context, req *GetStateRequest) (*StateResponse, error)
	TransactionalStateOperation(ctx context.Context, req *TransactionalRequest) error
	GetReminder(ctx context.Context, req *GetReminderRequest) (*Reminder, error)
	CreateReminder(ctx context.Context, req *CreateReminderRequest) error
	DeleteReminder(ctx context.Context, req *DeleteReminderRequest) error
	CreateTimer(ctx context.Context, req *CreateTimerRequest) error
	DeleteTimer(ctx context.Context, req *DeleteTimerRequest) error
	IsActorHosted(ctx context.Context, req *ActorHostedRequest) bool
	GetActiveActorsCount(ctx context.Context) []ActiveActorsCount
}

But state api, no context at all:

type Store interface {
	BulkStore
	Init(metadata Metadata) error
	Delete(req *DeleteRequest) error
	Get(req *GetRequest) (*GetResponse, error)
	Set(req *SetRequest) error
}

type BulkStore interface {
	BulkDelete(req []DeleteRequest) error
	BulkGet(req []GetRequest) (bool, []BulkGetResponse, error)
	BulkSet(req []SetRequest) error
}

I suggest to pass the context to all the components in golang style.

In fact, now I'm working to add tracting support in our state and bindings component, and I find that I can't get the context from the public API method of dapr gRPC service, I have to find other way (using metadata, again, we use metadata to do almost everything that dapr don't support) to pass the tracing information like traceid.

[skyao]

In service invacation, the context is passthrough the process and finally used for tracing:

	if IsGRPCProtocol(internalMD) {
		processGRPCToGRPCTraceHeader(ctx, md, grpctracebinValue)
	} else {
		// if httpProtocol, then pass HTTP traceparent and HTTP tracestate header values, attach it in grpc-trace-bin header
		processHTTPToGRPCTraceHeader(ctx, md, traceparentValue, tracestateValue)
	}

I think we should do similar in other components such state / bindings.

[skyao]

@yaron2 How about this ?

[yaron2]

@yaron2 How about this ?

Yes, that would be a good thing to do. With gRPC at least, we will be able to cancel ongoing calls.

There is a PR in place to add a context to Pub Sub.

[skyao]

@yaron2 How about this ?

Yes, that would be a good thing to do. With gRPC at least, we will be able to cancel ongoing calls.

There is a PR in place to add a context to Pub Sub.

OK, I will PR for bindings and state.

[artursouza]

Is anyone working on this issue?

/cc @skyao @yaron2

[ShaileshSurya]

@yaron2 @artursouza Is this still open. If it is can you help me with what all areas should I look into?

[artursouza]

@ShaileshSurya Look at the interfaces for the building blocks in dapr/components-contrib repo.

[ShaileshSurya]

@artursouza I see that the component-contrib/state is being consumed by the Dapr repository.

We might need to open two different PR's on component-contrib and dapr. This can cause both the repo's going out of sync for a while.

How can we go around this?

[artursouza]

@artursouza I see that the component-contrib/state is being consumed by the Dapr repository.

We might need to open two different PR's on component-contrib and dapr. This can cause both the repo's going out of sync for a while.

How can we go around this?

This is common. You work on both repos at the same time and submit the PR for both. After we merge the PR for contrib, the PR for dapr/dapr is updated with the latest contrib version in master and we merge that one too.

[pigletfly]

@skyao @artursouza I'd like to contribute to the bindings part.

[berndverst]

@pigletfly I reviewed your PR. I left instructions on your PR for next steps you must complete once the contrib PR is merged:

• open PR against dapr/dapr runtime.go (and update relevant tests) to set the context for the bindings methods, and also make sure to import latest components-contrib into dapr/dapr
• Once the dapr/dapr PR is merged, open PR to import the latest dapr/dapr into the certification test framework (the folder tests/certification within dapr/components-contrib) and fix all bindings tests there to also include the context as necessary.

[pigletfly]

@berndverst as for the pubsub component, should be change the interface into

type PubSub interface {
	Init(metadata Metadata) error
	Features() []Feature
	Publish(ctx context.Context, req *PublishRequest) error
	Subscribe(ctx context.Context, req SubscribeRequest, handler Handler) error
	Close(ctx context.Context) error
}