Skip to content

Log stdout to event log on failure to start process for ANCM out of process#10123

Merged
jkotalik merged 8 commits into
masterfrom
jkotalik/outOfProcBufferOutput
May 17, 2019
Merged

Log stdout to event log on failure to start process for ANCM out of process#10123
jkotalik merged 8 commits into
masterfrom
jkotalik/outOfProcBufferOutput

Conversation

@jkotalik
Copy link
Copy Markdown
Contributor

@jkotalik jkotalik commented May 9, 2019

Still thinking about how to simplify some event logs (kind of spammy right now). Will mark out of draft appropriately.

Fixes #10005

@jkotalik jkotalik requested a review from BrennanConroy May 9, 2019 22:12
@jkotalik jkotalik added this to the 3.0.0-preview6 milestone May 9, 2019
@jkotalik jkotalik marked this pull request as ready for review May 9, 2019 22:51
@jkotalik
Copy link
Copy Markdown
Contributor Author

This be green now 😄

Copy link
Copy Markdown
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a test that verifies only the first 30kb of output is logged?

Comment thread src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp Outdated
Comment thread src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp Outdated
Comment thread src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp Outdated
Comment thread src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp Outdated
Comment thread src/Servers/IIS/AspNetCoreModuleV2/CommonLib/resources.h Outdated
Comment thread src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp Outdated
Comment thread src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp Outdated
@jkotalik
Copy link
Copy Markdown
Contributor Author

Should there be a test that verifies only the first 30kb of output is logged?

Yeah we should. Laziness on my part 🐋

@jkotalik
Copy link
Copy Markdown
Contributor Author

🆙 📅

Comment thread src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp Outdated
@jkotalik
Copy link
Copy Markdown
Contributor Author

I would like to profile this before merging. I'm concerned that setting these to handles will have a perf impact if we are logging in a middleware.

@jkotalik
Copy link
Copy Markdown
Contributor Author

Verified locally, no performance regression when logging informational on localhost. I'll still like to do a second pass (and I'll add the accepted label eventually) after we merge this and it is available in the hosting bundle. I'm most concerned about performance on azure, so I'll make sure to verify it there.

I'll merge once this is green.

@jkotalik jkotalik merged commit 2e89aa4 into master May 17, 2019
@ghost ghost deleted the jkotalik/outOfProcBufferOutput branch May 17, 2019 23:31
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ANCM] [out of process] Write stdout to EventLog when exit code is non-zero

4 participants