Cold Start Optimization #10776
Open
Cold Start Optimization #10776
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jviau
reviewed
Jan 29, 2025
|
|
||
| foreach (string substring in ExcludedRequestSubstrings) | ||
| { | ||
| if (uri.IndexOf(substring, StringComparison.Ordinal) >= 0) |
There was a problem hiding this comment.
I think this will need to be made more robust:
- This looks like it might not be cheap to perform on a hotpath like this
- These substrings may have a chance of false-positives.
Possible solutions:
- Is there something besides URI on the
HttpRequestMessagewe can use to identify these calls? - Can we add something to the
HttpRequestMessageto identify these calls? Such as adding a value to.Optionsto indicate this call is to be filtered out. - Can we use an AsyncLocal to suppress Http client telemetry collection?
| return loggingBuilder | ||
| // These are messages piped back to the host from the worker - we don't handle these anymore if the worker has OpenTelemetry enabled. | ||
| // Instead, we expect the user's own code to be logging these where they want them to go. | ||
| .AddFilter<OpenTelemetryLoggerProvider>("Function.*", _ => !ScriptHost.WorkerOpenTelemetryEnabled) |
There was a problem hiding this comment.
Curious why we explicitly care about WorkerOptenTelemetryEnabled? What if worker is using app-insights. Don't ask why an app would use OTel in host and AppInsights in the worker, but what if they did?
What I am getting at is, should there be a general flag of worker is managing its own telemetry, and agnostic to exactly how it is doing so?
| @@ -7,6 +7,7 @@ internal enum TelemetryMode | |||
| { | |||
| None = 0, // or Default | |||
| ApplicationInsights = 1, | |||
| OpenTelemetry = 2 | |||
| OpenTelemetry = 2, | |||
| Placeholder = 3 | |||
There was a problem hiding this comment.
small nit: I would put placeholder as 1. Just feels right...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the Host gets initialized in placeholder mode, all needed assemblies get JIT compiled during placeholder mode so we will not pay the cost for JIT during Specialization. We want to exercise as much of the code path as we can during placeholder mode to avoid JIT during specialization.
resolves #10453
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.mdditional PR information