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

Lack of documentation in Prometheus.Inject #25

Open
hauleth opened this issue Oct 10, 2018 · 2 comments
Open

Lack of documentation in Prometheus.Inject #25

hauleth opened this issue Oct 10, 2018 · 2 comments

Comments

@hauleth
Copy link
Contributor

hauleth commented Oct 10, 2018

I would like to provide one, however it seems quite hard to deduce what is happening there.

@deadtrickster
Copy link
Owner

This one used to implement decorators, (see tests here https://github.com/deadtrickster/prometheus.ex/blob/master/test/injector_test.exs). For this it needs to walk over respective AST. Most of this file are workarounds for various syntaxes.

@hauleth
Copy link
Contributor Author

hauleth commented Oct 10, 2018

I think it would be better to stick to single form of decorating blocks instead. I would propose to support only function callbacks (0-arity), as these do not need to provide macros and are flexible enough to handle all possible cases. For example current implementation do not support functions with headers. You can test it by yourself with this diff:

diff --git a/test/injector_test.exs b/test/injector_test.exs
index ade3193..d9cdad7 100644
--- a/test/injector_test.exs
+++ b/test/injector_test.exs
@@ -31,6 +31,11 @@ defmodule Prometheus.InjectorTest do
       IO.puts("fun2")
     end
 
+    def fun_with_header(arg \\ 0)
+
+    def fun_with_header(arg) when is_binary(arg), do: IO.puts("fun_with_header binary")
+    def fun_with_header(_arg), do: IO.puts("fun_with_header other")
+
     Injector.test do
       def fun3() do
         IO.puts("fun3")

Or with private functions:

diff --git a/test/injector_test.exs b/test/injector_test.exs
index ade3193..b18d8a1 100644
--- a/test/injector_test.exs
+++ b/test/injector_test.exs
@@ -39,6 +39,10 @@ defmodule Prometheus.InjectorTest do
           IO.puts(e)
       end
     end
+
+    defp fun_p() do
+      IO.puts("fun_p")
+    end
   end
 
   def do_dangerous_work(x) do

And second one gives you quite unintuitive message:

== Compilation error in file test/injector_test.exs ==
** (RuntimeError) Mixing defs and other blocks isn't allowed
    (prometheus_ex) lib/prometheus/injector.ex:88: Prometheus.Injector.have_defs/1
    (prometheus_ex) lib/prometheus/injector.ex:41: Prometheus.Injector.inject_/2
    expanding macro: Injector.test/1
    test/injector_test.exs:25: Prometheus.InjectorTest (module)
    (elixir) lib/code.ex:767: Code.require_file/2
    (elixir) lib/kernel/parallel_compiler.ex:209: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/6

Because you have used def, just private one, why this one would not work?

So the only form available should be:

Prometheus.Injector.inject(wrapper, fn ->end)

While I would go with slightly different API, which is:

Prometheus.Injector.inject(callback, [before: &before_cb/0, after: &after_cb/1])

Where after_cb/1 would receive output of the before_cb/0.

Or alternatively remove everything, and implement simple macros/functions to measure behaviours in respective metrics, as this would be IMHO the easiest solution.

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