[release/7.0] Update usage of AllowEmptyTelemetry based on changes to the task in the SDK #82805
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…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 one thing that concerns me - it appears that runtime only has a 7.0 branch, but this change to the SDK Task is only valid from the 7.0.300 feature band. Since runtime is delivered to 7.0.100, 200, 300 and eventually 400 this change would error out in the earlier bands when the runtime updates in those channels. |
|
Tagging subscribers to this area: @hoyosjs Issue DetailsBackport of #82804 to release/7.0 Customer ImpactCustomers that build with 7.0.300 SDKs that trigger the Crossgen targets will have a compilation error because the shape of the SDK-delivered Task has changed. This is the minimal change to respond to the new shape. TestingThis change was manually verified by @MichaelSimons in #82795 (comment). RiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
|
@trylek I added the When/if this is ready, if you think this fix should be considered ask-mode, please add the |
|
@baronfel today's code-complete for servicing changes. In other words, last day to merge it so it goes into the April Release. I'm not sure if this is ready to merge or not. Can you help me answer the following questions?
|
carlossanlop
added
the
NO-MERGE
|
To be honest I'm not sure about many of those questions - mostly because I'm not sure if these targets are even in use in dotnet/runtime! I'm very much a dabbler here. If the targets aren't in actual use, and the ones from the SDK are the ones that we intend to use, then this entire thing could be removed. If it turns out these are actually in use by the runtime, then we have a much larger question - since the same runtime builds ship in all 7.x SDKs, the SDK cannot change the shape of the telemetry task (and any other tasks that are shared in this way) mid-cycle. That probably means we'd need to revert the change in the SDK in 7.x and reintroduce in 8. That's fine but slightly disappointing. The whole experience makes me want to do a thorough review of the tasks that dotnet/runtime use that are actually shipped in dotnet/sdk - so that this kind of thing isn't introduced again. |
|
Had a chat with @baronfel and @trylek, we feel confident to get this merged. Here are a couple of points provided by Chet:
|
carlossanlop
removed
the
NO-MERGE
ghost
locked as resolved and limited conversation to collaborators
Backport of #82804 to release/7.0
/cc @trylek @baronfel
Customer Impact
Customers that build with 7.0.300 SDKs that trigger the Crossgen targets will have a compilation error because the shape of the SDK-delivered Task has changed. This is the minimal change to respond to the new shape.
Testing
This change was manually verified by @MichaelSimons in #82795 (comment).
Risk
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.