Fix instance_ip_grouping_key not working on macOS #687
Conversation
Fixes prometheus#629 The solution is adapted from this StackOverflow answer: https://stackoverflow.com/a/28950776 Signed-off-by: Jethro Muller <[email protected]>
Signed-off-by: Jethro Muller <[email protected]>
|
Thank you for the PR, I just got back from a couple weeks of vacation, and will take a look at this soon. Just need to make sure I fully understand the workaround. |
There was a problem hiding this comment.
I apologize for the delay. I spent some time testing this, and my main concern is that it looks like the result could be different with this change. E.g. on my local machine instance_ip_grouping_key() changes from {'instance': '127.0.0.1'} to {'instance': '192.168.1.107'}. For many cases the new result probably makes more sense, but this could be a breaking change if someone is configuring their servers with a specific IP address.
|
Ah okay. That isn't good! I wasn't really sure how to test it and we didn't end up using the patch upstream as we had intended to when I made this PR. I guess this doesn't fit the bill then. I'm not really sure where to go from here so I guess I'll leave what happens next up to you. Thanks for the review 👍 |
|
One option would be to check if the platform is Darwin and use your new version in that case, but keep the old behavior for all other platforms? If there is a comment for why the two are handled slightly differently I would be happy moving forward with that. |
|
@csmarchbanks Cool, I can change the PR to do that 👍 |
Signed-off-by: Jethro Muller <[email protected]>
|
@csmarchbanks I've made the change to choose which method is used based on the platform 👍 |
Fixes #629
The solution is adapted from this StackOverflow answer: https://stackoverflow.com/a/28950776
@csmarchbanks I decided I'd just make the PR so long and if the solution isn't acceptable it can just be closed 👍