search

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

Allow for inet6 option #79

Closed cheerfulstoic closed 1 year ago

cheerfulstoic commented 1 year ago

Fixes #78

I'm using this to be able to send metrics over IPv6 on fly.io through WireGuard

cadebward commented 1 year ago

This doesn't seem to work for me. :inet.gethostbyname still fails when looking up a host on ipv6.

I think it needs to be updated to also take in the :inet6 option here https://github.com/beam-telemetry/telemetry_metrics_statsd/blob/main/lib/telemetry_metrics_statsd.ex#L547 (and a few lines up).

:inet.gethostbyname(host, :inet6) seems to work

cheerfulstoic commented 1 year ago

👍 Thanks for pointing this out! I was using IPv6 addresses directly instead of a hostname so that's probably why I didn't run into this.

To keep things cleaner I took away the inet6 option and replaced it with a inet_address_family option which can be either inet (the default), inet6, or local which is what is used by the inet:address_family() type. :udp.open/2 doesn't use inet:address_family() but rather allows specifying an atom for the option that you want to use, so I adapted to use that as best as I could.

I'm on holiday right now, so I won't get back to testing this right away, but I can test it with my project soon-ish.

cadebward commented 1 year ago

I tested it out, works great!

iamvery commented 1 year ago

Thanks for working through this. Would love to see this change released in the official package.

cheerfulstoic commented 1 year ago

I forgot to update that I've been using this change in my project for a week or so now and it's been working fine 👍

ludwikbukowski commented 1 year ago

@arkgil what do you think about this one? Can we merge ?

cheerfulstoic commented 1 year ago

Saw that CI failed. Just ran mix format which should hopefully help some. Not sure what the other errors are...

arkgil commented 1 year ago

@cheerfulstoic we'll need to wait until #82 and #83 are merged in order to merge this one.

cheerfulstoic commented 1 year ago

👍 Sure, I can add a test. I wasn't really sure how to do that, but I just dug in a bit. I guess I would use the given_udp_port_opened / assert_reported helpers, but maybe adjust them a bit to allow for defining the address family. Let me know if that seems off in some way

cheerfulstoic commented 1 year ago

I added a test, and I think it's probably the right thing, but let me know if you think it should work differently 😄

arkgil commented 1 year ago

@cheerfulstoic ideally we'd test that reporting works e2e with IPv6, so publishing a metric and using assert_reported in that test would be great!

arkgil commented 1 year ago

Also, if you could rebase on latest main, we could run tests for this PR. Thanks to @mopp CI is working again 💪

cheerfulstoic commented 1 year ago

Thanks very much! I've added a separate test to match the pattern of the other tests (i.e. :telemetry.executes in the top-level tests and hostname resolutions inside of the describe). It seems to fail when I disable my change

cheerfulstoic commented 1 year ago

(oh, and I rebased 👍 )

arkgil commented 1 year ago

Thank you for the contribution @cheerfulstoic. This change has been published in version 0.7.0 🎉