Update capabilities and log worker metadata after specialization #9020
Conversation
src/WebJobs.Script.Grpc/azure-functions-language-worker-protobuf/src/proto/FunctionRpc.proto
Outdated
Show resolved
Hide resolved
kshyju
force-pushed
the
shkr/env_reload_response_for_spcln
branch
from
0752ad0 to
0379800
Compare
| - Update App Service Authentication/Authorization on Linux Consumption from 1.4.17 to 1.5.1 [Release Note](https://github.com/Azure/app-service-announcements/issues/406) | ||
| - Upgraded proto files to v1.7.1-protofile release (#9023) |
There was a problem hiding this comment.
This one is not part of this PR. I forgot to include release_notes update in 9023.
| workerMetadata.UpdateWorkerMetadata(_workerConfig); | ||
| var workerMetadataString = workerMetadata.ToString(); | ||
| _metricsLogger.LogEvent(MetricEventNames.WorkerMetadata, functionName: null, workerMetadataString); | ||
| _workerChannelLogger.LogDebug("Worker metadata: {workerMetadata}", workerMetadataString); |
There was a problem hiding this comment.
This may not be applicable for the scenario here but wanted to call out that for one of my previous PRs - refer, I got to know that adding log lines here has perf impact. So, we can double check if this log is really needed here and not affecting the perf.
There was a problem hiding this comment.
This logging was existing code, which I moved to a private method now. So this PR should not make a perf impact as such.
But I get your point. We can definitely discuss removing this logging entry as we have this information getting logged to functions metrics as well. Thoughts @liliankasem ?
There was a problem hiding this comment.
I'm not sure if a detector or other teams are using the information from the log somewhere so I can't say it's 100% safe to remove
| @@ -395,7 +397,7 @@ internal void WorkerInitResponse(GrpcEvent initEvent) | |||
| UpdateCapabilities(_initMessage.Capabilities); | |||
|
|
|||
| _isSharedMemoryDataTransferEnabled = IsSharedMemoryDataTransferEnabled(); | |||
| _cancelCapabilityEnabled = !string.IsNullOrEmpty(_workerCapabilities.GetCapabilityState(RpcWorkerConstants.HandlesInvocationCancelMessage)); | |||
| _cancelCapabilityEnabled ??= !string.IsNullOrEmpty(_workerCapabilities.GetCapabilityState(RpcWorkerConstants.HandlesInvocationCancelMessage)); | |||
There was a problem hiding this comment.
What if the value of this capability is set to a false-y string? false, 0, etc
There was a problem hiding this comment.
Also, this may not be part of this PR but can we update the name _cancelCapabilityEnabled to may be _invocationCancelCapabilityEnabled or something on similar lines. Only cancel capability is not communicating the meaning till I saw RpcWorkerConstants.HandlesInvocationCancelMessage
There was a problem hiding this comment.
Currently, host code(in various places) checks the existence of the capability instead of the value being truthy. I agree, Ideally it should be a truthy check. but that might involve coordinating with workers to make sure they send the correct value.
Currently dotnet worker sends capability when the value is truthy. Ex: https://github.com/Azure/azure-functions-dotnet-worker/blob/14cc88859a1b7bbf68ee43409a1ad45a6e8362cb/src/DotNetWorker.Grpc/GrpcWorker.cs#L222-L229
| _workerChannelLogger.LogDebug("Worker metadata: {workerMetadata}", workerMetadata); | ||
| } | ||
| // In placeholder scenario, the capabilities and worker metadata will not be available | ||
| // until specialization is done (env reload request). So these can be removed from worker init response code path. |
There was a problem hiding this comment.
We should check our understanding here with @soninaren as I remember change in flow on placeholder side for the work - selective loading of extensions.
This is a second set of changes on host side to address #8983. This changes follows the proto file changes made in Azure/azure-functions-language-worker-protobuf#90
In placeholder & specialization use case, the worker metadata and capabilities are available only after specialization (env reload response). In this PR, Updating the code path which handles env reload request to refresh the capabilities and log the worker metadata.
Pull request checklist
release_notes.mdAdditional information