fix: properly handle remote hostname #30
Conversation
|
Yeah you guessed right. The tool does that often it seems :) About the extension method reorder, I'd noticed something earlier. If the |
|
Yes you are correct. I always forget that :-( I wish they'd let us somehow define the method(s) used when configuring using app settings. |
|
Yeah took me a while to figure out as well. Maybe I'll submit a patch to fix their reflection logic to do a best match. |
|
That would surely help most sink developers! |
|
I'll have to quit for now, but will continue with this tomorrow. Have a good night. |
| <ItemGroup> | ||
| <None Update="appsettings_output_template.json"> | ||
| <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | ||
| </None> | ||
| <None Update="appsettings_text_formatter.json"> | ||
| <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | ||
| </None> | ||
| <None Update="appsettings_remote_ip_address.json"> |
|
I'm going to finish up the last changes tomorrow, and then we'll merge the PR. Thank you for your patience. |
|
Super, I'll submit another PR for the IPv6 mapped socket type. There's no reason to create IPv4 sockets when dual stacks are already supported at the OS level :) |
|
IPv6 has gained a lot of ground, but there are still environments where this isn't available by default (source), I would rather probe the hostname and investigate its address family, and then setup the UDP client using that family, or is your argument that the dual mode always will work? |
|
Exactly, the dual mode always works via IPv4 translation (since IPv4s can be represented as IPv6s) |
|
I was worried about Windows XP, but since XP doesn't support .NET 4.5 which I build for, it seems that we are in the clear. You can open up a new PR after this one is merged, which will be tonight I think. Thank you for all effort you've put into this issue. |
|
You're welcome, appreciate the help with the clean up. PS: Also when using hostnames, IPv4/IPv6 check is of little significance since a DNS record can hold both types. |
Fixes issue where the hostname only resolves into an IP address when the sink is created. The very nature of hostnames is to provide a route to instances whose IP address may change, and these code changes allows for this situation. One limitation with the changes is that currently only IPv4 addresses are supported, but an upcoming PR will solve that issue. Closes FantasticFiasco#29
FantasticFiasco
force-pushed
the
master
branch
from
aa29cd5 to
6fec234
Compare
|
@FantasticFiasco I noticed that you've removed the existing I'd like to state that this can potentially impact performance for known IP address cases since they would be first sent to a DNS resolver and then converted to IPEndpoint. The DNS resolution being the critical thing here - subject to implementation - could cause potential issues (in most cases should be negligible). If that's acceptable then no further action is needed. Also it's unfortunate that the PR was rebased on my master and not during merge time - so we've lost the history in case we wish to go back. |
|
I've looked at the |
|
I've updated the code, you can see the changes here. |
|
If you wish to recover the lost commits, I have the branch still locally - now with lots of merge conflicts so would reflog it but if you can work it out that's also okay. For next time best use Squash and merge of GitHub or squash only the PR. My version of fork is now out of sync :) |
|
Sorry I messed up your history, wont happen again :-( I guess this is one on the reasons why GitHub proposes to create pull requests from branches. |
|
No worries. I'll work it out. Usually one would squash merge the PR on GitHub directly leaving the original branch intact - it's a bit different from |
|
Its a good day when you learn something new, thanks! |
|
You're welcome. I'll work on the mapped IPv4 PR and you might wanna look into upgrading to the latest Serilog.Settings.Configuration package when this is merged serilog/serilog-settings-configuration#131 Have a nice evening. |
fixes #29