Enable periodic hostname resolution

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

Enable periodic hostname resolution #48

Closed haljin closed 3 years ago

haljin commented 3 years ago

Resolves #45 (at least partially)

Make TelemetryMetricsStatsd optionally resolve the hostname of the Statsd server on startup and then again on a specified interval
Allows for dynamic reconfiguration of the host/port of the reporter
Additionally optionally allows for naming of the ETS pool allowing for more reporters to be started at the same time without name conflicts

haljin commented 3 years ago

The check is failing due to an optional dependency between :meck and :hamcrest - it seems the older Dialyzer does not figure out that this is not being used here. I guess the solution is to ignore that warning? I don't really know how else to test this feature without using mock :).

arkgil commented 3 years ago

Thanks @haljin, periodic DNS resolution looks like a great way forward! I'd remove the function to update the hostname manually for now.

Would you be able to test how this improves performance in your system before we merge?

haljin commented 3 years ago

I've tried it now and I can see quite a big difference, e.g.:

exec_time is time reported by Redis itself, while transaction_time includes client time + sending 3-4 metrics to StatsD. Considering it's only a couple a drop of 2 ms on average is quite significant. I'm also assuming the metrics will be less spiky now too, however to fully validate it I'd have to update all the services to make sure all metrics sent by everyone are using IPs.

Another service that calls this one can also see it respond much faster (about 25% improvement)

arkgil commented 3 years ago

Thanks @haljin, this looks very promising!

juanperi commented 3 years ago

This PR is incredible!! I had this situation where I couldn't send ecto telemetry, because it was slowing everything down. I thought it was because of the single genserver, so I sent the PR for using a pool. It didn't fix the issue. I stopped digging there, but honestly never thought about the DNS resolution as being the culprit. Now, It's been 15 minutes running this PR in production... and the slowness doesn't seem to appear anymore.

Kudos @haljin!! this is awesome

arkgil commented 3 years ago

@epilgrim thanks for posting this! Thanks @haljin! 🙏