App Insights Tags for Orchestrators and Entities

[davidmrdavid]

This addresses: #1172

This PR aims to add custom App Insights dimensions to enable querying recent orchestrators and entity calls.

For Orchestrators, we add the following fields:

• DurableFunctionsType as "Orchestrator"
• DurableFunctionsInstanceID
• DurableFunctionsStatus

For Entities, we add the following fields:

• DurableFunctionsType as "Entity". I believe this tag name is a confusing, I'm open to improvements. Overall, it was unclear to me if these tags should also be prefixed with DurableFunctions- or DurableEntity
• DurableEntityInstanceID
• DurableEntityStatus

I added these tags at the beginning of the "execute" methods, but if mid-execution their state is changed, then maybe this is not a good idea. Please let me know what you think. Overall, the current approach is rather naive so I expect to iterate on this.

Also, I realize that DurableEntityStatus returns a string-version of what are really JSON-codified fields. I'd be happy to expand those into proper tags as well.

Update 1:
Below is a screenshot of how the changes look for Entities. I'm keep the screenshot small as to avoid leaking any sensitive information. This is the counter entity from the precompiled samples.

[cgillum]

I had the same question about adding this information before or after the actual function execution. I think the right way to go is to do this at the end of the execute method so that we have the right status value.

BTW, were you able to verify that this data gets correctly populated in App Insights? It would be great if you could provide some screenshots in your PR description showing what this looks like.

[sebastianburckhardt]

Now that I see the context where the information shows up, I think including the entire GetStatus() result for the entity status is probably overkill. Perhaps going back to something simple that just shows the state size as suggested by @cgillum is better. This also avoids the overhead of allocating multiple objects and calling JSON serialization on every entity operation.

For example

activity.AddTag("DurableEntityStatus", this.context.State.EntityExists ? $"has state ({this.context.State.EntityState.Length} characters)" : "no state");

As noted by others this should be done at the end of the operation to make sense (so it displays the final state, not the state before the operation).

[anthonychu]

I'm open to suggestions about the prefix. However it feels like things that are the same between orchestrations and entities (like instance id) should share the same key.

As for what we output as statuses for entities, outputting nothing is an option as well. We can always add later.

The goal of this is to be able to list a summary of the last x orchestrations or entities that ran. Someone can click on a single instance id and the portal will render more details from the traces table using a query similar to this.

Should ...ID be ...Id to match other places in code?

[anthonychu]

Maybe we should also match the naming convention to those we use already here: https://docs.microsoft.com/en-us/azure/azure-functions/durable/durable-functions-diagnostics#single-instance-query

e.g., DurableFunctionsStatus should be DurableFunctionsState.

[cgillum]

Some small comments.

[cgillum]

It looks like the App Veyor build failed because the C# compiler issued a warning when compiling against the .NET Framework (i.e. Functions v1). Basically, to the compiler it looks like you created a variable and assigned a value but never used it (we only use it for .NET Standard/Functions v2). A couple possible solutions:

1. Use #if for all usage of this variable
2. Suppress the warning for this specific instance.

I'm personally fine with either of these options. I'm open to other ideas as well.

[davidmrdavid]

@cgillum , Yup! I finally figured out how to run the AppVeyor style cop on locally on Visual Studio, so those surprises should not be happening anymore. Fixed it using your first suggestion

[davidmrdavid]

Summary of the latest changes:

• To facilitate querying, I made Entities and Orchestrators share the same keys. A user should be able to filter between them by checking on the value of DurableFunctionsType
• Unfortunately, I cannot use probe the OrchestrationRuntimeState returned by dispatchContext.GetProperty<OrchestrationRuntimeState>() as a reliable way to report a DurableFunctionsRuntimeStatus. In my experience, that value was always set to "Running", which is unhelpful.
  • As a result, you'll see that my current approach simply tracks the TraceHelper calls into a variable and uses that to report a value instead.
  • In general, it would be worthwhile to explore why that value is always set to "Running" and whether or not we're in need of some refactoring as a result. Additionally, I also found the string retuned by OrchestrationRuntimeState.Status to be always null, adding to my concerns.

• I'll have a picture of the results in Azure App Insights posted shortly

[ConnorMcMahon]

Looks good for the most part. There is a lot of conditional compilation that I think we could probably reduce to make this much easier to read.

[davidmrdavid]

This is how the changes reflect on App Insights:

Query:

requests
| extend DurableFunctionsInstanceId = tostring(customDimensions["DurableFunctionsInstanceId"]),
         DurableFunctionsStatus = tostring(customDimensions["DurableFunctionsRuntimeStatus"]),
         DurableFunctionsType = tostring(customDimensions["DurableFunctionsType"])
| summarize arg_max(timestamp, *) by DurableFunctionsInstanceId
| order by timestamp desc
| limit 30

I don't know where that first empty row is coming from, but it shouldn't be coming from my changes as I have "Orchestrator" or "Entity" hard-coded forDurableFunctionsType

[ConnorMcMahon]

Pushing this to our next release.

[reyang]

LGTM with minor suggestions.

[sebastianburckhardt]

LGTM.