Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macro idea for async instrumentation: >defn-go #24

Open
tslocke opened this issue Apr 1, 2019 · 6 comments
Open

Macro idea for async instrumentation: >defn-go #24

tslocke opened this issue Apr 1, 2019 · 6 comments
Milestone

Comments

@tslocke
Copy link

tslocke commented Apr 1, 2019

I have a lot of core.async in my (CLJS) app. I guess spec + core.async is a very big topic, and at some point the Clojure community will probably come up with a powerful approach. In the mean time, this is more of a low-hanging-fuit idea that could be very helpful.

In async code there are a lot of functions where the top level form is (go ...). Often the return value is discarded, but sometimes not, e.g. an async function to fetch a value from a server via HTTP.

The normal return spec is pretty much useless, as such functions always return a channel.

What you really want is to be able to spec the (single) value that is sent over the returned channel. From an instrumentation point of view, this could be done if we lift the (go ...) to be part of the macro that defines the function (and the specs), i.e. something like this:

(>defn-go my-fn 
  [...args]
  [...arg-specs => ret-spec]
  ...body)

which, when instrumented, would expand to something along the lines of

(defn my-fn
  [...args]
  ; check passed args against arg-specs (as usual)
  (go
    (let [result# ...body]
      ; check result# against ret-spec
      result#)))

I'm intending on having a crack at implementing this macro myself. Any advice would be greatly appreciated. If it sounds like something that could be a contribution, so much the better.

@tslocke tslocke changed the title Macro idea for async instrumentation:>defn-go Macro idea for async instrumentation:>defn-go Apr 1, 2019
@tslocke tslocke changed the title Macro idea for async instrumentation:>defn-go Macro idea for async instrumentation: >defn-go Apr 1, 2019
@gnl
Copy link
Owner

gnl commented Apr 3, 2019

This sounds interesting. A couple of quick thoughts off the top of my head:

  • At the moment Ghostwheel is only meant to be used during development and any spec-related code is stripped in production. I assume you'd want to use this in production?
  • You'd probably want to use the ^::g/outstrument metadata to enable checking on the fn input and output, but instead of simply doing spec-instrumentation with orchestra as it is usually done in Ghostwheel, you'd do regular spec-instrumentation to check the input and implement the output check as described above.
  • This sounds like it might be better to put into a separate opt-in module that depends on the main ghostwheel module, similar to https://github.com/gnl/ghostwheel.specs for example. This way I wouldn't have to change the general policy of "everything ghostwheel-related is gone in production" if I decide to add this to the project and if I don't, you could simply have a separate ghostwheel module without having to fork and maintain the whole thing.

@tslocke
Copy link
Author

tslocke commented Apr 3, 2019

The wrapping of the body of the fn with (go ...) would have to remain in place in production, but I was thinking in terms of the normal behaviour for the instrumentation part - only have that code generated for development, and when :outstrument true is configured. It looks like I would need something like this in my macro def, to get at that config, right?

(let [{:keys [::g/outstrument]} (g/merge-config (meta fn-name))]
  ...)

I was also thinking >defn-go would be implemented in terms of >defn, so I'd only have to add the outstrument part.

Yes I'd have to disable the regular "outstrumenting" with orchestra, as the regular return value (a channel) would fail the spec. I can set that in the metadata on fn-name before calling defn>.

@gnl gnl added this to the 0.5.0 milestone Jul 14, 2019
@gnl
Copy link
Owner

gnl commented Jul 14, 2019

I think I wanna have something like this in there—async is obviously hugely important—but I'll have to do some more thinking and a bit of exploratory programming to be able to comment on this any further and it'll probably be a while because right now I'm focused on getting 0.4.0 out the door. I'm setting the milestone to 0.5.0 for now.

@tslocke
Copy link
Author

tslocke commented Jul 14, 2019

Your comment has prompted me to get back to this and post a simple macro I've started using. Right now it's CLJS only, and doesn't support nil in place of the spec vector. May well have other issues but I'm using it and getting some value out of it.

Other than the nil spec issue it's basically a drop-in replacement for >defn. As discussed you get an implicit (go ...) around your function body, so you mustn't also have an explicit one.

I added a bit of a hack to make failure messages more informative. Instead of emitting s/assert with a literal spec, the macro emits an (s/def ::my-ns/my-function-name-return ...). The failure message will then lead you directly to the offending function.

   (defmacro >defn-go [fn-name & fn-tail]
     (let [front-stuff (take-while (complement vector?) fn-tail)
           rest        (drop (count front-stuff) fn-tail)
           [args spec & body] rest
           spec*       (concat (drop-last spec)
                               [`any?])
           ret-spec    (last spec)
           ret-spec-kw (keyword (str ana/*cljs-ns*) (str (name fn-name) "-return"))]
       `(do
          (cljs.spec.alpha/def ~ret-spec-kw ~ret-spec)
          (ghostwheel.core/>defn ~fn-name ~@front-stuff ~args [~@spec*]
            (go (cljs.spec.alpha/assert ~ret-spec-kw (do ~@body)))))))

@gnl
Copy link
Owner

gnl commented May 21, 2020

👋 Hi. I'm posting this same comment to all issue threads to just give a quick heads up that the project, despite rumours and some evidence to the contrary, is not dead. It was hibernating for a little while and now nearing the long-awaited next release, which will fix some long-standing issues (and introduce some breaking changes to the config).

I'll be reviewing all open issues and PRs over the next couple of weeks, so stay tuned and thanks for the patience.

See also this Slack thread

@gnl
Copy link
Owner

gnl commented Jul 20, 2023

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

No branches or pull requests

2 participants