-
Notifications
You must be signed in to change notification settings - Fork 3
Add Invocation Id to Module Private Data #99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -45,6 +45,15 @@ public void SetDurableClient(object durableClient) | |||
_hasSetOrchestrationContext = true; | |||
} | |||
|
|||
public void SetInvocationId(string invocationId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would I actually call this to set the invocation id? I tried to find how SetDurableClient()
is getting called, but didn't find it in this SDK.
I looked at the internal SDK implementation and I see that it gets called in DurableController.cs
_powerShellServices.SetDurableClient(durableClient);
Do I need to update the Functions PowerShell worker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe you would need to move this method to the PowerShell worker and invoke it from there. In fact, the SetDurableClient
and SetOrchestrationContext
methods do not make sense in this repo, and they were probably just copied over from the worker repo by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But wait, this will require us to deploy a PS worker. Perhaps we can find a way to avoid that. Two ideas:
- Ask the customer to invoke
Set-FunctionInvocationContext
explicitly from their app code. Make this a part of DT enabling instructions (at least temporary, until we are able to update the worker). Do you know if the invocation id is available in the app code? - Perhaps we can make the OTel module invoke
Set-FunctionInvocationContext
automatically? @andystaples, any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we decided that the invocation id should come from the OTel module? In this case, we can bypass the worker completely by either making the OTel module invoke Set-FunctionInvocationContext
directly, or make the Durable SDK module invoke a command from the OTel module, whatever is more convenient. These modules don't have to take a hard dependency on each other, they will just know that cmdlets with certain names may exist, so this will be a dynamic dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, the SetDurableClient and SetOrchestrationContext methods do not make sense in this repo, and they were probably just copied over from the worker repo by mistake.
Hmm, I seem to recall there was a reason - I recall Michael Peng telling me about this class at some point, but to be frank the reasons escape me now. My suggestion - add print statements / obvious failures in this codepath and run a few tests. If indeed it doesn't get called, then yeah just remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Ask the customer to invoke Set-FunctionInvocationContext explicitly from their app code. Make this a part of DT enabling instructions (at least temporary, until we are able to update the worker). Do you know if the invocation id is available in the app code?
The invocation ID is available in the function through $TriggerMetadata
for client functions so we could use this approach for now. If we use this approach, then the customer would just have to call Set-FunctionInvocationContext
? Then, we could get the invocation ID in the Start-DurableOrchestration
function?
param($Request, $TriggerMetadata)
$invocationId = $TriggerMetadata.InvocationId
Write-Host "Invocation ID: $invocationId"
- Perhaps we can make the OTel module invoke Set-FunctionInvocationContext automatically? @andystaples, any thoughts on this?
@andystaples, would this work or would we still run into the issue where we don't have the invocation ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can make this work - if the PowerShell worker detects the presence of the OTel module, it will call Start-OpenTelemetryInvocationInternal to do some setup. This cmdlet takes the invocation ID as the parameter, so we can call Set-FunctionInvocationContext from here.
Ordering-wise - you can review this code here: https://github.com/Azure/azure-functions-powershell-worker/blob/bd1bfa65da3ab4c22f427dadce024b5d5a730384/src/PowerShell/PowerShellManager.cs#L218
but essentially, the PowerShell worker queues up a series of cmdlets to run immediately before the user code in order to set state. Currently, the order is
- Set-FunctionInvocationContext -DurableClient
- Import-Module
- Start-OpenTelemetryInvocationContext
I don't think there should be an issue calling Set-FunctionInvocationContext -InvocationID in step 3.
The change to azure-functions-powershell-opentelemetry would look something like this (with better error handling, probably) Azure/azure-functions-powershell-opentelemetry@74f5e22
/// The function invocation ID. | ||
/// </summary> | ||
[Parameter(Mandatory = true, ParameterSetName = InvocationIdKey)] | ||
public string InvocationId { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be careful with this parameter. Unless we make the customer invoke this cmdlet explicitly, we need to remember that whatever invokes this cmdlet may be dealing with an older SDK version that didn't have this parameter yet. So the attempt to invoke it will fail, and we need to make sure that the caller tolerates the error. Nothing special to do in this PR, but please keep this in mind.
This PR adds support to add Invocation Id information to Module Private Data so we can use it for distributed tracing in
Start-DurableOrchestration
.This is a draft PR to discuss the design. It will be updated to get tracing information from the Durable PowerShell OpenTelemetry SDK based on Invocation Id in the
Start-DurableOrchestration
function.