set subscription id and tenant id in telemetry if it's null #427
Conversation
isra-fel
left a comment
isra-fel
left a comment
There was a problem hiding this comment.
PR itself looks good.
If we do this, does it make the logic of setting sub id in AzureRMCmdlet.BeginProcessing duplicate, and can we remove that?
Also, this will impact cmdlets that do not inherit AzureRMCmdlet . Please ensure we have tested that scneario. Thanks.
@isra-fel , according to your comment, I changed the update and test the below cmdlets. Their subscription ids and tenant ids are recorded correctly now.
|
|
|
||
| IAzureContext context; | ||
| _qosEvent.Uid = "defaultid"; | ||
| if (RequireDefaultContext() && TryGetDefaultContext(out context)) |
There was a problem hiding this comment.
Need more time to check if it's safe to remove RequireDefaultContext. Will catch up next week
There was a problem hiding this comment.
@YanaXu can you test running some cmdlets that doesn't require a context after clearing the context? They should still work without a context.
Examples are the *DataCollection and ContextAutoSave cmdlets.
isra-fel
left a comment
isra-fel
left a comment
There was a problem hiding this comment.
PR looks good if we verified the above cmdlets
It's for
Content-AzAccount. When it's run, the subscription id and tenant id is not set in context. That's why they are empty in telemetry.This PR is going to set them before recording the telemetry.