Update usage of AllowEmptyTelemetry based on changes to the task in the SDK #82804
Conversation
…he SDK In dotnet/sdk#30269 the API of the AllowEmptyTelemetry task changed to allow for more granular hashing of the collected telemetry properties. This change was against a servicing branch and flowed into SDK main. Sometime after that change, these targets moved from SDK to Runtime, and so the change was lost. This re-applies the change to Runtime main to address breaks seen in source-build.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
@trylek what's the procedure for doing backports in this repo? This is my first runtime PR :) |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4298771340 |
|
@baronfel - the backport PR should be ready now, please see above, just "edit" the PR description and fill in the info, that should do the trick. Please make sure to include my M2 @jeffschwMSFT as a reviewer, he is typically overseeing the logistics of the servicing process. |
|
Oh, one more detail I've stumbled upon before - once the backport PR has been approved, don't actually merge it, that's going to be done once the servicing chores open a window for such a thing, typically @carlossanlop will ultimately see to it. |
|
this also needed to go to 8.0 preview 2 according to #82795 (comment) - does that also happen with the backport action? |
Unfortunately, we're already past code complete for Preview 2: https://dev.azure.com/devdiv/DevDiv/_wiki/wikis/DevDiv.wiki/31692/NET-8-Schedule How critical is this fix? @mmitche can tell you if this change can still go into Preview 2 or not. |
|
We are very far behind on p2 (given the shortened schedule), so this is unlikely to make it. |
|
@baronfel - we're approaching the deadline for accepting contributions to the next NET 7.0 servicing release but this change in runtime main hasn't yet been merged in, as a matter of fact it's nowhere near green; can you please investigate the remaining failures and / or comment on their potential infra causes so that we can make a case for the backport? |
|
At least some of the failures were just general flakiness (IIRC the macOS runners where heavily degraded when the PR originally went up). There are two remaining that consistently are failing, but I don't see a connection between this and the 62 reported test failures. @trylek could you sanity check the checks and see if there's anything that immediately jumps out to you? |
|
@baronfel - I don't see any failures in the PR that would be related to your change, most of the failures I saw actually look like a temporary network failure, I think this PR should be safe to merge. If you want to be super sure, you can "retry the failed legs" using the AzDO UI, I would expect transient failures to go away. |
In dotnet/sdk#30269 the API of the AllowEmptyTelemetry task changed to allow for more granular hashing of the collected telemetry properties. This change was against a servicing branch and flowed into SDK main. Sometime after that change, these targets moved from SDK to Runtime, and so the change was lost.
This re-applies the change to Runtime main to address breaks seen in source-build.
Is there a layering concern here with consuming an SDK-delivered Task? Is the crossgen telemetry still relevant and should we still be collecting it?