Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Account for AddressFamily pairs in NameResolution telemetry #56614

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jul 30, 2021

In #56208 I changed BeforeResolution to take an object instead of string/IPAddress, but the value we feed it from RunAsync could also be KeyValuePair<string, AddressFamily> or KeyValuePair<IPAddress, AddressFamily>.

Type check for those as well (same as we do elsewhere).

string h => GetHostEntryCore(h, AddressFamily.Unspecified, stopwatch),
KeyValuePair<string, AddressFamily> t => GetHostEntryCore(t.Key, t.Value, stopwatch),
IPAddress a => GetHostEntryCore(a, AddressFamily.Unspecified, stopwatch),
KeyValuePair<IPAddress, AddressFamily> t => GetHostEntryCore(t.Key, t.Value, stopwatch),

@MihaZupan MihaZupan added this to the 6.0.0 milestone Jul 30, 2021
@MihaZupan MihaZupan requested a review from a team July 30, 2021 10:27
@ghost
Copy link

ghost commented Jul 30, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

In #56208 I changed BeforeResolution to take an object instead of string/IPAddress, but the value we feed it from RunAsync could also be KeyValuePair<string, AddressFamily> or KeyValuePair<IPAddress, AddressFamily>.

Type check for those as well (same as we do in elsewhere).

string h => GetHostEntryCore(h, AddressFamily.Unspecified, stopwatch),
KeyValuePair<string, AddressFamily> t => GetHostEntryCore(t.Key, t.Value, stopwatch),
IPAddress a => GetHostEntryCore(a, AddressFamily.Unspecified, stopwatch),
KeyValuePair<IPAddress, AddressFamily> t => GetHostEntryCore(t.Key, t.Value, stopwatch),

Author: MihaZupan
Assignees: -
Labels:

area-System.Net

Milestone: 6.0.0

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephentoub
Copy link
Member

Do we need another test?

@karelz
Copy link
Member

karelz commented Jul 30, 2021

BTW:
This is a 100% Debug.Assert in the product which is merged for 3.5 days:

It is super-weird that it failed on @MihaZupan PR (which introduced the problem), but all checks there were green:

  • The likely reason is that Outerloop was kicked off on 7/23 @ 15:36 GMT+2 and a typo-fix commit in 45 minutes (7/23 @ 16:21 GMT+2) kicked off another run which did NOT include any results from previous Outerloop run.

The other problem is that this 100% Outerloop problem was in the product unnoticed for 3 days -- the PR which introduced it was merged #56208 (comment) on 7/26 @ 21:03 GMT+2 ... yet we noticed it only today around 7/30 @ 11 GMT+2 by accident on another Networking PR which ran Outerloop.

  • Querying Kusto, there is no Outerloop failure in Official run (that is very concerning)
  • Querying Kusto is super-hard ... if we didn't what to look for, there is no chance we would have noticed from queries that something is wrong

cc @dotnet/dncenghot I will start email thread to see what we can do about these things longer-term

@MihaZupan
Copy link
Member Author

Do we need another test?

The telemetry tests do exercise all overloads, so the assert should be hit in every outerloop run as Karel noted. It's just unfortunate it took this long to spot it.

@MihaZupan
Copy link
Member Author

Outerloop tests unrelated: #56618 #54260 #55313

@MihaZupan MihaZupan merged commit 58232e0 into dotnet:main Jul 30, 2021
@mmitche
Copy link
Member

mmitche commented Jul 30, 2021

@karelz You can see the results of the previous run (that completed) here: https://github.com/dotnet/runtime/runs/3144425881 (when you look at the checks tab it shows results from all commits in a drop-down.

What happened here was completely by design on the AzDO side. When the new commit was pushed, a couple things happen:

  • The currently executing runs against the previous source commit are cancelled automatically by AzDO.
  • The newly triggered runs are associated against the new commit in github.

The likely reason is that Outerloop was kicked off on 7/23 @ 15:36 GMT+2 and a typo-fix commit in 45 minutes (7/23 @ 16:21 GMT+2) kicked off another run which did NOT include any results from previous Outerloop run.

Note that even if the runs continued and completed, they would not show up in the GitHub Checks (unless you dig through the history). This is because Github checks are associated with the source sha, and the head source sha of the PR changed on the new push. The previous results are not included because they aren't the same code.

So what really needed to happen in this case would be for the outerloop run to re-launch on the new sha. Today, as far as I know, there is no way to do that in an automated fashion. Without that, the run could have been re-requested after the push.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants