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

small qol update to :telemetry.attach/4 docs #126

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ethangunderson
Copy link
Contributor

This has come up a few times for us. It's not always obvious that the function supplied to attach must be /4. When supplying something else you get an exception message that also doesn't reveal the problem.

iex(2)> :telemetry.attach("test", [:test, :event], fn -> IO.inspect("Hello") end, nil)
** (FunctionClauseError) no function clause matching in :telemetry.attach_many/4

    The following arguments were given to :telemetry.attach_many/4:
        # 1
        "test"

        # 2
        [[:test, :event]]

        # 3
        #Function<43.3316493/0 in :erl_eval.expr/6>

        # 4
        nil

    (telemetry 1.2.1) /Users/egunderson/Library/Caches/mix/installs/elixir-1.14.2-erts-13.1.2/e123f7ef1d039f8d998d1a6a03fb9b81/deps/telemetry/src/telemetry.erl:106: :telemetry.attach_many/4
    iex:2: (file)

I'm hoping that adding a slightly more explicit explanation of the function argument in function doc will help some people avoid this confusion in the future.

@whatyouhide
Copy link
Contributor

I think it might be worth improving the error message too. In Elixir, we've often done this by removing the arity from the is_function check. In this case, that is, use is_function(Fun) instead of is_function(Fun, 4). With this, when you try to call the function with a wrong arity, the error message is a lot easier to debug 😉

@ethangunderson
Copy link
Contributor Author

@whatyouhide I like that suggestion. Do you want me to make that change in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants