Port to release/2.2 branch: EventPipe support for "Parameters" #20796

[jorive]

No description provided.

[jorive]

@dotnet-bot test this please

[noahfalk]

LGTM

[Eilon]

Are there any new/updated tests required for this change?

[vancem]

This is a port of #20796 in Main to the 2.2. branch.

[vancem]

@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test
@dotnet bot test OSX10.12 x64 Checked Innerloop Build and Test

[Eilon]

I'm a bit worried that this significant feature has no additional test coverage. Are there tests somewhere else for this feature? Manual tests run by vendors?

[vancem]

Jose has done manual testing. He can give you details. (basically turned on an EventCounter both before the app launched and after it was running and confirmed it worked, I also suggested doing a multi-counter tests).

We also agree that we need real tests and that they are part of the work item. They will be coming (ideally they would have been part of this checkin). We are only in this place right now because we are bumping into a deadline.

I too do not like the fact that we are checking in without tests. What makes it OK for me is that there is coverage for the existing features, and the new feature really is pretty simply (pass a string through to existing logic (EventSource/EventCounters) that already know what to do with it (and have tests in the ETW scenario). We also did the manual test that mimics the important scneario (turning on EventCounters), end to end.

So yes, we are asking to check in without tests, but we will fix that shortly.

[vancem]

@Eilon @DamianEdwards - I have a question: The ARM CI seems to be down right now (I have a query in to fix it). Is there a deadline we want to hit and if so should we merge this without those tests or should we wait?

[Eilon]

Understood about the tests. BTW there's still a short window (say, end of next week), for test-only changes (no product changes at all without approval).

If we are reasonably confident with the change as it is now, let's go ahead and get it checked in today, so that we can try to get builds ready for weekend vendor testing. If we later find an issue with ARM machines, we can revisit. It'll depend on how broken it is. If it just doesn't work on ARM, and the fix is expensive, then maybe that's just too bad and we'll fix in 3.0.

[Eilon]

(But if ARM all of a sudden is completely broken in unrelated scenarios, that's bad.)

[vancem]

@Eilon - The best case of course is to get the CI fixed. I am working on that. The question is when do we not wait for that. It sounds like we can wait until late today is that correct?

For what it is worth, an ARM only issue is very unlikely. This change is a port from Main, and ARM tests passed there. With any luck at all, however, this will be fixed by noon.

[mmitche]

@dotnet-bot test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test
@dotnet bot test OSX10.12 x64 Checked Innerloop Build and Test

[Eilon]

Earlier is better 😄

Our dear expert @mmitche probably knows best when "late" becomes "too late."

[mmitche]

Tizen failure is expected because corefx doesnt' have a corresponding tizen build

[jorive]

@Eilon As Vance mentioned, I did some manual testing:

• dropping the file before process start
• dropping the file after process start
• deleting the file
• multiple event sources / counters

I agree it is not ideal not having testing enabled with this PR, but we are committed to adding tests in the immediate future.

[vancem]

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test

[mmitche]

@vancem the Tizen job will not pass because there is no corresponding corefx job, and the old coreclr test workflow required a corefx build to work.