trailblazer / trailblazer-operation

Trailblazer's Operation implementation.
https://trailblazer.to/2.1/docs/operation.html
87 stars 24 forks source link

DRAFT: extract ciruit_options when using public call interface #65

Closed skorbut closed 11 months ago

skorbut commented 11 months ago

When providing circuit options while using the public call interface, these should be taken into account.

In order to do this we filter out known circuit options before merging all into the options.

Open question: Is there a definitive source for circuit_options keys?

Also I noticed, when using call with the circuit interface the circuit options are picked up, but get lost later in the process. See added specs in call_test.rb

apotonick commented 11 months ago

Hi Karsten, thanks for looking into this! Have you seen this commit where I try to fix that very problem? https://github.com/trailblazer/trailblazer-operation/commit/ac2fa2b667674032c38b68e37860cbad54e865ab

There is no defined set for circuit_options, unfortunately!

apotonick commented 11 months ago

BTW, when using the public interface you're not supposed to provide circuit_options! The signature of Operation.call may suggest that, but you may only pass ctx and optional flow_options. Could you tell us why you need the public interface in combo with low-level options? :beers:

skorbut commented 11 months ago

Hi,

we currently have several operations that were implemented quite optimistically. In order to add more detail when reporting failed operations I'd like to supply a protocol that will upload to our error reporting tool. In this protocol I list all steps with its result and runtime. The protocol is created using a TaskWrap extension and added to every operation via some BaseOperation class were I override the call method like

def self.call(*args, **kwargs)
    puts "BaseOperation#call"
    # register protocol extension
    protocol_extension = Trailblazer::Activity::TaskWrap::Extension(
      [::OperationsProtocol.method(:start_task),  id: "protocol.start_task",  prepend: "task_wrap.call_task"],
      [::OperationsProtocol.method(:end_task), id: "protocol.end_task", append: "task_wrap.call_task"],
    )
    protocol_wrap = Hash.new(protocol_extension)
    ctx = kwargs || {}
    byebug
    #super([ctx, {}], wrap_runtime: protocol_wrap)
  end

I also tried switching from public interface to circuit interface here, but it didn't call the extension. It's like the spec I added in this PR for testing the circuit interface with wrap_runtime.

apotonick commented 11 months ago

This looks like you should definitely switch to the circuit interface: Operation.([{ctx}, {flow_options}], **your_options).

My plan has been (for a long time) to make as many users as possible invoke an endpoint that then calls the operation with appropriate options. The public Operation.call interface was a short-sighted idea to encourage service objects.

Can you check if the above signature works for you? Alternatively, try using the private method directly https://github.com/trailblazer/trailblazer-operation/blob/master/lib/trailblazer/operation/public_call.rb#L69

skorbut commented 11 months ago

Can you have a look at this spec? It is basically the same as the public interface spec, but it fails. Somehow the extension doesn't get called. If I get this to succeed, I wouldn't need the public interface change.

apotonick commented 11 months ago

Sorry I was busy with the new website, I will figure out where we lose your :wrap_runtime tomorrow!

skorbut commented 11 months ago

Thanks a lot, new Website looks great.

apotonick commented 11 months ago

@skorbut This is how you make it work when using Operation.call:

signal, (ctx, _) = operation.call(
      [{seq: []}, {}],
      wrap_runtime: Hash.new(my_extension),
      runner: Trailblazer::Activity::TaskWrap::Runner
    )
apotonick commented 11 months ago

Note that the ctx won't be automatically converted to a Context as it's done here: https://github.com/trailblazer/trailblazer-operation/blob/master/lib/trailblazer/operation/public_call.rb#L79

This is why I vote for slowly fading out usage of Operation.call in the next years in favor of invoking operations using an endpoint.

skorbut commented 11 months ago

Thanks a lot for looking into this, just ran a few tests and it looks good. I'm closing this PR, no need to change a deprecated interface.

apotonick commented 11 months ago

Well, technically, it's not deprecated as we cannot simply remove Create.(params: ...) :grimacing: the questions here are

  1. how do we manage to get people use the circuit interface for low-level concerns?
  2. what form of an endpoint would encourage developers to use that abstraction instead of globally overriding private methods to inject arbitrary data into the operation call?