Emit metrics when requesting retry/reboot #8187
Conversation
|
@MattGal would you care to have a look at this one? I made this after your suggestion and it was a really good idea since it will give us a lot of useful signals (see the linked issue for details) |
| @@ -157,8 +157,10 @@ def analyze_operation(command: str, platform: str, device: str, isDevice: bool, | |||
|
|
|||
| print(f"Reporting {len(operations)} events from diagnostics file `{diagnostics_file}`") | |||
|
|
|||
| # Parse operations, analyze them and send them to Application Insights | |||
| retry_dimensions = None | |||
There was a problem hiding this comment.
what value do these provide? Why not just use custom_dimensions in all metric sending?
There was a problem hiding this comment.
So imagine you run a sequence of XHarness operations:
xharness android devicexharness android installxharness android run<- let's say this one fails because the device is lockedxharness uninstall
These are processed in the for loop, the 3rd one is identified as a reason to retry/reboot.
Below, outside of the for loop, we want to report the retry with as much info as possible.
Some of the operations might have less info (which device was used, which target..) so we want to pin the one operation that gets us the retry and take its dimensions and use it with the retry/reboot.
I will maybe just use its exit code as the metric value though.
There was a problem hiding this comment.
I am maybe not following, so you view this as something for the future where you'll have other reboot / retry dimensions? If so, I'd suggest you just add that when such a thing is possible. I don't see any possible way for the values to vary based off which stage of device/install/run/uninstall we're at.
There was a problem hiding this comment.
I'm not sure I follow?
I need to emit only one retry/reboot event per work item. I want the event to have some custom dimensions so that I can have a query such as "give me the number of reboots on iOS 14.2 devices". This is just an example and I'm not sure exactly what queries will be useful in the end but why not have the data with the event?
There was a problem hiding this comment.
Are you proposing I emit it the retry event when setting the retry variable to true?
There was a problem hiding this comment.
I'm just saying from my reading of the code you're always just sending custom_dimensions with no modifications, so lines 211 and 215 could just use custom_dimensions. I assume there's some future extensibility reason for it and I don't see anything that wouldn't work here so I'll just approve and assume you have some purpose in mind or I'm not seeing something :).
No objections here, but I don't see why you declare new variables. |
| @@ -157,8 +157,10 @@ def analyze_operation(command: str, platform: str, device: str, isDevice: bool, | |||
|
|
|||
| print(f"Reporting {len(operations)} events from diagnostics file `{diagnostics_file}`") | |||
|
|
|||
| # Parse operations, analyze them and send them to Application Insights | |||
| retry_dimensions = None | |||
There was a problem hiding this comment.
I'm just saying from my reading of the code you're always just sending custom_dimensions with no modifications, so lines 211 and 215 could just use custom_dimensions. I assume there's some future extensibility reason for it and I don't see anything that wouldn't work here so I'll just approve and assume you have some purpose in mind or I'm not seeing something :).
|
Custom dimensions doesn't live outside of that for loop though. And I don't want to get the last one that was set in the loop because at least for Apple it won't have enough data (it will be uninstall most likely) |
Resolves https://github.com/dotnet/core-eng/issues/14930