beam-telemetry / telemetry_metrics_statsd

Telemetry.Metrics reporter for StatsD-compatible metric servers
https://hexdocs.pm/telemetry_metrics_statsd
MIT License
75 stars 44 forks source link

Small doc fix #7

Closed haljin closed 5 years ago

haljin commented 5 years ago

Just noticed this small mismatch in the docs. Also is there any reason for

  defp format_metric_value(%Metrics.Counter{}, _value), do: "1|c"

So basically the value of the counter is always ignored?

arkgil commented 5 years ago

@haljin Hi 👋

Yes, the value of the counter is ignored, because the counter as defined by Telemetry.Metrics is supposed to aggregate the number of events.

If you need to sum up specific measurements, then you can use the sum metric.

I agree that this is not best explained in the docs, though. This example was supposed to highlight that indeed, for counters the event measurements are always ignored. Maybe we should add a sentence or two about it somewhere near that place in the docs.

haljin commented 5 years ago

Right now I get it. I'll reword it slightly. I'm also a bit confused how tags interact with statsd. The way we used Statix was to append the hostname of the kubernetes pod to the metric itself (so we could see if it is e.g. a single pod misbehaving) but I can't see if it even is possible to do with Telemetry?

I tried

 @spec metrics :: [Telemetry.Distribution.t()]
  def metrics do
    [
      distribution("storage.transaction", tags: [:hostname], buckets: [0])
    ]
  end
@spec timing(:telemetry.event_name(), (() -> any())) :: any()
  def timing(metric, fun) do
    {timing, result} = :timer.tc(fun)
    :telemetry.execute(metric, %{request: timing}, %{hostname: hostname()})
    result
  end

  defp hostname do
    {:ok, hostname} = :inet.gethostname()
    hostname
  end

but that doesn't seem to work?

arkgil commented 5 years ago

@haljin that's strange, it should. The way StatsD reporter works with tags is that it appends a tag value to the metric name, in your case it should be "storage.transaction.".

What metric name do you see on the StatsD side?

haljin commented 5 years ago

I figured it out, I was missing the request suffix. I'll update the PR with some more suggestions on how to make it a bit clearer :)

arkgil commented 5 years ago

@haljin yeah, you always need to specify the measurement. That is defined by Telemetry.Metrics, not the reporter, so I'm not sure how to go about documenting it. I'd like to avoid duplicating documentation in both projects, so I'm wondering if we shouldn't link to the Metrics docs at the top of reporter's documentation.

haljin commented 5 years ago

That would probably also help. But maybe a few more examples wouldn't hurt either. Maybe I'm just being slow today :) I guess feel free to just close this then.

arkgil commented 5 years ago

@haljin I'll keep this open, feel free to push any ideas you have, as I'm biased to how understandable the documentation is :D

haljin commented 5 years ago

I'll close this for now as I don't really have any more suggestions at the moment. If anything pops up I will make a new one. :)