Allow for inet6 option

[cheerfulstoic]

Fixes #78

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

[cadebward]

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]

👍 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]

I tested it out, works great!

[iamvery]

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

[cheerfulstoic]

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]

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

[cheerfulstoic]

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

[arkgil]

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

[arkgil]

Thanks for the PR @cheerfulstoic! The code looks good, but we'll need a test for it too.

[cheerfulstoic]

👍 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]

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]

@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]

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

[cheerfulstoic]

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]

(oh, and I rebased 👍 )

[arkgil]

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