Use Module for configuring formatter instead of function ref

[andyleclair]

Hi!
We've found that when attempting to build releases using Distillery, it barfs when trying to use a function ref as a config value.

This PR adds a behaviour so that anyone who wants to add a custom formatter module can do so safely.

[sourcelevel-bot]

Hello, @andyleclair! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

[thecodeboss]

This looks like a wonderful change, thanks @andyleclair. One concern I have is that this breaks compatibility if others have been using the formatter.

Could you also keep the old behaviour by adding definitions with is_function(formatter) guards? That way users would have a choice to use a module that implements the behaviour, or a function.

[andyleclair]

@thecodeboss that's a great idea. Give me a few minutes

[andyleclair]

@thecodeboss how's that look to you?

[andyleclair]

there aren't really any formatting tests, but I guess I could add some if you want!

[thecodeboss]

You can give the parameters names in this typespec to make it a bit more readable:

@type metric_type :: :counter | :gauge | :histograms | :spirals | :timer
@callback format(metric_type(), metric_name :: String.t()) :: [String.t()]

Also I think the specis incorrect, the first parameter is an atom instead of a string.

[thecodeboss]

how's that look to you?

Looks great!

there aren't really any formatting tests, but I guess I could add some if you want!

I think that would be helpful, since it's not obvious what a formatter should look like (ie. what is it supposed to return?). Could also help to add a simple example of a formatter in the docs

[andyleclair]

I don't really see a great way to test this that wouldn't be just testing that guards and @behaviour work, but I've added some more docs to the readme and the recommended typespec. Also, not sure if that spec failure is transient, they work for me locally?

[thecodeboss]

I think this should be a more concrete example instead of just a comment. For example

def format(_metric_type, name) do
  # appends a constant prefix to metric names, and replaces dashes
  # with underscores
  ["my_prefix", String.replace(name, "-", "_")]
end

[thecodeboss]

Also, not sure if that spec failure is transient, they work for me locally?

Not sure what spec failure you're referring to.

Also I think this is okay without tests. Note I'm not one of the maintainers though so I won't be able to merge for you.

[jparise]

Should Elixometer.clear_counter/1 also use the formatter? It's currently calling name_to_exometer/2 directly.

elixometer/lib/elixometer.ex

Line 292 in a5e6f75

clear_counter(name_to_exometer(:counters, metric_name))

... and if so, then I think we can move the name_to_exometer/2 logic into the default format/2 function and remove name_to_exometer/2 entirely.

[andyleclair]

@jparise I did consider that, but it's only working on the internal state of the GenServer so I decided to leave it as is. That's why the default Formatter just calls name_to_exometer/2

I don't think it's a bad idea though, I did consider it.

[jparise]

I'm not very familiar with this code, but it looks like Elixometer.update_counter/3 calls Updater.counter/3, which ultimately uses the formatted metric name. I would expect Elixometer.clear_counter/1 to need to format its metric name the same way in order for it to successfully clear the previously stored counter.

Unless I have that wrong, that's an existing bug.

[andyleclair]

@jparise updated!