feat(taskbroker): Enable querying for namespace specific tasks

[enochtangg]

Overview:
This PR is responsible enabling taskbroker to receive a namespace parameter for the worker client and return pending activations specific for that namespace.

Dependencies:

• feat(taskbroker): Add namespace parameter to get/set requests sentry-protos#68 (will need to release and bump version in order to pass CI)
• feat(taskbroker): Improve fetch_next sentry-protos#69

Refs:

• Implement the namespace parameter for GetTask #58

Testing:

• Unit test added below
• Tested end-to-end with taskworker

[markstory]

Having to provide both fetch_next and fetch_next_namespace creates a trap where providing fetch_next_namespace in isolation will be silently ignored. Perhaps we need two cases, one for fetch_next and one for fetch_next_namespace?

[john-z-yang]

+1, would it make more sense to redefine fetch_next using a richer type?

Like

fetch_next: Option<Fetch>

Enum Fetch {
    Arbitrary,
    ByNamespace(String)
}

I'm not sure how to express this in protobuf and how it will translate via prost, maybe something like

message GetTaskResponse {
    ...
    optional fetchNext fetchNext = 1;
}

message fetchNext {
    oneof fetchBy {
        string namespace = 1;
        google.protobuf.NullValue arbitrary = 2;
    }
}

[markstory]

A oneof could work, I don't think we would need the nested message either.

message SetTaskStatusRequest {
  oneof fetch_next_by {
      string fetch_namespace = 6;
      bool fetch_any = 7;
  }
}

We might also be able to adapt the existing fields into a oneof as oneof doesn't use up slots in the proto.

[john-z-yang]

I see, so with

message SetTaskStatusRequest {
  oneof fetch_next_by {
      string fetch_namespace = 6;
      bool fetch_any = 7;
  }
}

If use caller does not want to fetch another message, they would set the fetch_any variant to false?

[markstory]

If use caller does not want to fetch another message, they would set the fetch_any variant to false?

Yeah, it can also be left empty (at least in the python bindings).

message SetTaskStatusRequest {
  string id = 1;
  TaskActivationStatus status = 3;

  // Set to true to have receive a new task in the response
  optional bool fetch_next = 4;
  optional string fetch_next_namespace = 5;

  oneof fetch_next_by {
      string fetch_namespace = 6;
      bool fetch_any = 7;
  }
}

And usage of the generated python code

from sentry_protos.sentry.v1.taskworker_pb2 import SetTaskStatusRequest
from sentry_protos.sentry.v1.taskworker_pb2 import TASK_ACTIVATION_STATUS_COMPLETE

>>> req = SetTaskStatusRequest(id="abc", status=TASK_ACTIVATION_STATUS_COMPLETE)
>>> req.fetch_any
False
>>> req.fetch_namespace
''
>>> req = SetTaskStatusRequest(id="abc", status=TASK_ACTIVATION_STATUS_COMPLETE, fetch_any=True)
>>> req.fetch_any
True
>>> req = SetTaskStatusRequest(id="abc", status=TASK_ACTIVATION_STATUS_COMPLETE, fetch_any=True, fetch_namespace="derp")
>>> req
id: "abc"
status: TASK_ACTIVATION_STATUS_COMPLETE
fetch_namespace: "derp"

>>> req.fetch_any
False

It doesn't seem like the generated code will raise an error if both are provided, but only one value is set into the object.

[john-z-yang]

Ah okay, personally a oneof with a string/bool variant is a little less intuitive than an optional oneof with a string/unit variant for some reason. But I think either is serviceable and it seems like the former is more easily expressed in protobuf.

[enochtangg]

What if we just use made the fetch_next message itself optional, and use that to replace fetch_any. It feel like it could be a bit more intuitive than using oneof at all?

For example:

message FetchNext {
  optional string namespace = 1;
}

message SetTaskStatusRequest {
  string id = 1;
  TaskActivationStatus status = 2;

  // If fetch_next is provided, receive a new task in the response
  optional FetchNext fetch_next = 3;
}

as defined in getsentry/sentry-protos#69

[john-z-yang]

That works too!

[markstory]

We should also update the worker in sentry to use the undeprecated parameters.

[markstory]

You might be able to use QueryBuilder to reduce the duplication in these queries. You could initialize the query with the query up to the namespace condition, and then use .push() and .push_bind() to add the namespace condition and limit/returning clauses.

[enochtangg]

Updated, using query builder 👍 Though, I find it a bit less legible because query builder requires use to build it piecewise

[markstory]

Yeah, I miss having an ORM style builder. But I also don't think we have enough complex queries to justify a larger library yet.

[markstory]

Looking good.

[markstory]

This passage looks the same on both sides of the match arms, could it be moved down? You'd have to move the result assignment down as well though.