Capture request body aspnet core by katzdan · Pull Request #402 · elastic/apm-agent-dotnet

katzdan · 2019-07-21T08:22:48Z

Closes #213

initial feature commit -
Added config values + behavior in the Apm middle-ware class.
Errors body extraction still pending.

@SergeyKleyman , @gregkalapos
As there are differences between the node.js and java docs - Iv'e kept the implementation to whatever is common :

the accepted values in 'captrueBody' config value can be 'off', 'all', 'errors' or 'transactions'
the request body, when extracted, is truncated after 2kb.
please let me know if anything else is required for this feature.

I added tests to verify this works along side a test that creates a 'post' http call ad in order for that to work properly I added a new controller that accepts post requests to the 'SampleAspNetCoreApp' project. There are other controllers that have post actions but these redirect the UI to another page and that would have made the tests more complicated.

There is one part of the implementation I don't know how to do - when the 'captureBody' is set to 'errors' than the apm module should retrieve the body from the request but there is no HttpContext in the error class or anywhere in the classes that handle this scenario. I'll need some guidance with this.

initial feature commit - Added config values + behavior in the Apm middle-ware class. Errors body extraction still pending.

Had to make minor changes to the code due to some name changes.

katzdan · 2019-07-21T11:13:06Z

FIY - all the tests related to new feature run successfully in my machine (there is one that fails - register user test which expects a web service that's not running ). I'm not sure why there are conflicts. I have the latest version of master locally, branched of course, but still - up to date. There shouldn't be any conflicts.

gregkalapos · 2019-07-21T11:27:36Z

@katzdan thanks for the PR!

To address your question:

There is one part of the implementation I don't know how to do - when the 'captureBody' is set to 'errors' than the apm module should retrieve the body from the request but there is no HttpContext in the error class or anywhere in the classes that handle this scenario. I'll need some guidance with this.

Maybe you already figured this out, but just in case:
The Error uses the same Context instance as the current transaction. Long term it'd be probably better to not share the same instance, but let's ignore it now - that's not directly related top this PR anyway. So, this Context instance has a Request property and if you look at the Request type you'll see that it's the same one that you already use.

There are 2 ways when we create errors in ASP.NET Core.

when ASP.NET Core fires an Microsoft.AspNetCore.Diagnostics.UnhandledException or ..HandledException event, we handle this here This happens when there is an error handler page registered.

As you can see in this screenshot you have access to the HttpContext there, similar to the exception you just need to read it with reflection:

the other case is when our middleware catches the exception here This happens when there is no error handler page registered in the pipeline.

Now one thing you can already do is that before we catch the error you simply set the transaction.Context.Request.Body - in both cases you have the HttpContext, I guess this won't bee too much code.

A nicer way would be to give you access to the error before it's sent out, but that'd require more refactoring, and I think it'd better to do that in a separate PR. So probably the option described above would be more feasible.

Hope this helps.

SergeyKleyman · 2019-07-21T11:49:19Z

I recommend to test use cases with

Chunked transfer encoding
HTTP compression - it's mostly used for responses but it seems it can be used for requests as well
Both Chunked transfer encoding and HTTP compression

By testing I don't imply we have to capture request body in these use cases (it's worth syncing with other agents) - we should have tests verifying whatever is decided agent should do in these use cases.

gregkalapos · 2019-07-21T11:57:14Z

I'm not sure why there are conflicts. I have the latest version of master locally, branched of course, but still - up to date. There shouldn't be any conflicts.

I'm not sure this is the case. I think your branch does not have #374. If you compare e.g MicrosoftExtensionsConfig.cs on your branch and on master you'll see it's different - those are mostly just auto code cleanups, so things got moved, but no logical changes. (so my cleanup causes you trouble 👼 ) - also if I look at the history I see your branch misses some commits. Try merging origin/master to your branch again - or rebase from it.

katzdan · 2019-07-21T13:06:30Z

Thanks @SergeyKleyman , @gregkalapos , I'll implement according to your guidelines and report back here.

gregkalapos

I went through the code, added some comments.
Most of them are just small things, but the exception I mention is something which we should think about. Other than those, in my view this definitely goes the right direction. 👍👍

Thanks for your effort @katzdan!

sample/SampleAspNetCoreApp/Controllers/ValuesController.cs

src/Elastic.Apm.AspNetCore/ApmMiddleware.cs

test/Elastic.Apm.Tests/ConfigTest.cs

gregkalapos · 2019-07-21T13:43:34Z

src/Elastic.Apm/Config/AbstractConfigurationReader.cs

+			var captureBodyInConfig = kv.Value;
+			if (string.IsNullOrEmpty(captureBodyInConfig))
+			{
+				return ConfigConsts.SupportedValues.CaptureBodyOff;


nit: why not return DefaultValues.captureBody here, as below?

In order not have to deal with a null/empty value passed in the config file or env variables. Let me know if you prefer to change that. I'd feel better to align the code to the preference of the devs with more experience in this project.

I'm not sure I can follow you.

So, this is the part I talk about:

if (string.IsNullOrEmpty(captureBodyInConfig)) { return ConfigConsts.SupportedValues.CaptureBodyOff; }

All I suggest is to replace ConfigConsts.SupportedValues.CaptureBodyOff with DefaultValues.captureBody, because in case the entered value is null or an empty string we want to return the default value - which is indeed the same as ConfigConsts.SupportedValues.CaptureBodyOff, it just communicates the intent better.

So in this sense... we do deal with null/empty values: we simply return the default in that case.

OK. Will do.

Done. code is pushed.

gregkalapos · 2019-07-21T13:49:18Z

src/Elastic.Apm.AspNetCore/ApmMiddleware.cs

+			{
+				using (var reader = new StreamReader(context.Request.Body))
+				{
+					body = reader.ReadToEnd();


I see some exceptions here when I run the sample app:

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1] An unhandled exception has occurred while executing the request. System.IO.IOException: Unexpected end of Stream, the content may have already been read by another component. at Microsoft.AspNetCore.WebUtilities.MultipartReaderStream.ReadAsync(Byte[] buffer, Int32 offset, Int32 count, CancellationToken cancellationToken) at Microsoft.AspNetCore.WebUtilities.StreamHelperExtensions.DrainAsync(Stream stream, ArrayPool`1 bytePool, Nullable`1 limit, CancellationToken cancellationToken) at Microsoft.AspNetCore.WebUtilities.MultipartReader.ReadNextSectionAsync(CancellationToken cancellationToken) at Microsoft.AspNetCore.Http.Features.FormFeature.InnerReadFormAsync(CancellationToken cancellationToken) at Microsoft.AspNetCore.Mvc.ModelBinding.FormValueProviderFactory.AddValueProviderAsync(ValueProviderFactoryContext context) at Microsoft.AspNetCore.Mvc.ModelBinding.CompositeValueProvider.CreateAsync(ActionContext actionContext, IList`1 factories) at Microsoft.AspNetCore.Mvc.ModelBinding.CompositeValueProvider.CreateAsync(ControllerContext controllerContext) at Microsoft.AspNetCore.Mvc.Internal.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<<CreateBinderDelegate>g__Bind|0>d.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted) at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync() at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync() at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext) at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context) at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context) at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

I googled this and it seems to be a common thing. This one suggest disabling model binding, but that does not help in our case - we can't tell this to all our users.

I'm trying to help here and come up with something. I'd say this is definitely something where we should come up with a solution for.

Looking into this as well.

@gregkalapos - I wasn't able to reproduce this problem (please provide instructions on who to do it). However - the problem of an "already read" request/response stream is a well known one. There are several approaches to resolve this , with the one most appropriate for this problem being

Resetting the body stream reader to position 0 when it's supported.

Reading the body and then writing it back to the memory to prevent subsequent attempts to the read the body from failing.
I'm implementing and testing it now.

There are other solutions which cannot be used for various reasons -

EnableRewind() for all requests in the pipeline but that would mean we are changing the behavior of the web app on the server.

Request.EnableBuffering() would have resolved this problem but is only available from .Net core 2.1.

refs :
https://gunnarpeipman.com/aspnet/aspnet-core-request-body/
http://www.palador.com/2017/05/24/logging-the-body-of-http-request-and-response-in-asp-net-core/
https://devblogs.microsoft.com/aspnet/re-reading-asp-net-core-request-bodies-with-enablebuffering/

Code committed. Please check on your end if possible or provide instructions for recreating this problem.

How do you test? I simply sent an HTTP POST to http://localhost:5000/api/Home/Post with Postman. If I remember correctly, HTTP GET requests from a browser did not repro the problem for me either (I should have said this earlier.)

To the current version: I still see some problems, and this time it happens always:

Application started. Press Ctrl+C to shut down. fail: Elastic.Apm[0] {ApmMiddleware} General Error reading request body , Error msg : Specified method is not supported. , stack trace : at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.set_Position(Int64 value) at Elastic.Apm.AspNetCore.Extensions.HttpRequestExtensions.ExtractRequestBody(HttpRequest request, IApmLogger logger) in /Users/gergelykalapos/Repos/apm-agent-dotnet/src/Elastic.Apm.AspNetCore/Extensions/RequestExtentions.cs:line 28 fail: Elastic.Apm[0] {ApmMiddleware} General Error reading request body , Error msg : Specified method is not supported. , stack trace : at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.set_Position(Int64 value) at Elastic.Apm.AspNetCore.Extensions.HttpRequestExtensions.ExtractRequestBody(HttpRequest request, IApmLogger logger) in /Users/gergelykalapos/Repos/apm-agent-dotnet/src/Elastic.Apm.AspNetCore/Extensions/RequestExtentions.cs:line 28 ^CApplication is shutting down...

It's very strange that on this post no one mentions this problem.

Doesn't this happen on your machine?

This GitHub issue is the same: aspnet/KestrelHttpServer#1548

And the solution for that is to call request.EnableRewind(); - that's currently not available, for that we need a reference to the Microsoft.AspNetCore.Http package. But it's still very strange that on the 1. blogpost people seem to suggest that as an alternative, and no one says that the original version without request.EnableRewind(); does not seem to work. It defiantly fails on my machine and the referenced GitHub issue seems to verify that without request.EnableRewind(); it should not work.

@gregkalapos - I tested using the SampleAspNetCoreApp and with test methods (
TestJsonBodyRetrievalOnRequestFailureInAspNetCore, FailingPostRequestWithoutConfiguredExceptionPage, HomeSimplePagePostTransactionTest, all in AspNetCoreMiddlewareTests.cs).
I'll try it your way. The test methods for this worked but since I merged master into my working branch - these tests fail. It looks like the initialization of the agent in the test method (with the captureBody setting being set to all/errors/transactions) is being ignored and then debugging I can see that the loaded value in the website being used in the test is "off". I'm still trying to understand why this is happening.

I removed the request.body.position=0 statement after re-creating the problem.
Still working on the 'Unexpected end of Stream' problem (Which I haven't been able to recreate yet).

katzdan · 2019-07-22T17:25:55Z

I recommend to test use cases with

Chunked transfer encoding

HTTP compression - it's mostly used for responses but it seems it can be used for requests as well

Both Chunked transfer encoding and HTTP compression

@SergeyKleyman - I started to go over the resources you mentioned and will comment with some recommendations.

@SergeyKleyman - The Apm agent Java implementation supports collection of UTF-8 encoded plain text content types. It would make sense to do the same with the dotnet agent.
I don't think it makes sense to log binary/chunked/compressed content.
As a first step - we should limit the request body capturing to the java agent default : application/x-www-form-urlencoded*, text/, application/json, application/xml*.

Let me know what you think.

This commit includes the following : 1. Errors monitoring now supports the 'captureBody' feature + tests. 2. Values Controller is removed, the Post methods for testing have been moved to the Home Controller. 3. Refactoring : Functionality of the request body retrieval has been moved to an extension method class ('RequestExtensions.cs'). 4. Added more tests. 5. Small changes to captureBody default values in tests.

1. Added request body capturing in the ApmMiddle InvokeAsync method. 2. Added a test to verify this scenario. 3. Refactored some of the code that determines if the agent should capture the request body for better readability and maintainability.

katzdan · 2019-07-24T09:05:51Z

@gregkalapos - I added the second case of error handling in ApmMiddleware.cs.
@SergeyKleyman - Please comment on which content-types should be supported.
aside from that - I'll handle the file conflicts later today (@gregkalapos - I tried what you proposed but it didn't resolve the situation, I'll have to try other solutions).

SergeyKleyman · 2019-07-24T09:11:44Z

@SergeyKleyman - Please comment on which content-types should be supported.

I recommend to follow Java agent approach - https://www.elastic.co/guide/en/apm/agent/java/current/config-http.html#config-capture-body-content-types

Please note that Content-Type of HTTP request is orthogonal to both aspects I've mentioned before (Chunked transfer encoding and HTTP compression).

SergeyKleyman · 2019-07-24T09:26:48Z

@katzdan If you arrive to conclusion that supporting Chunked transfer encoding and/or HTTP compression is a lot of additional effort we can declare those use cases as not supported for now and see if there is any demand for them from the customers.

katzdan · 2019-07-24T12:51:24Z

@SergeyKleyman - indeed, I think that supporting text content is enough for now. Supporting these cases would increase the amount of code here considerably. I think it would make sense to align with the Java agent and add support for non text content types in the future based on customers' input.

gregkalapos · 2019-07-25T14:41:31Z

@SergeyKleyman - indeed, I think that supporting text content is enough for now. Supporting these cases would increase the amount of code here considerably. I think it would make sense to align with the Java agent and add support for non text content types in the future based on customers' input.

Yeah, I'd also take this approach.

(@gregkalapos - I tried what you proposed but it didn't resolve the situation, I'll have to try other solutions).

Regarding the conflicted changed: It seems you resolved those 👍

gregkalapos

@katzdan - adding some other small things I saw in the code.

src/Elastic.Apm.AspNetCore/ApmMiddleware.cs

gregkalapos · 2019-07-25T14:52:59Z

src/Elastic.Apm.AspNetCore/ApmMiddleware.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.IO;


nit: this is unused - it can be removed.

I did a file clean-up in my last commit.

src/Elastic.Apm.AspNetCore/Extensions/RequestExtentions.cs

gregkalapos · 2019-07-25T14:59:54Z

src/Elastic.Apm.AspNetCore/Extensions/RequestExtentions.cs

+			} catch (IOException ioException){
+				logger.Error()?.Log("IO Error reading request body , Error msg : {exception} , stack trace : {stacktrace} " , ioException.Message , ioException.StackTrace);
+			} catch (Exception e) {
+				//TODO : remove this after havning finalized all cases and testing all scenarios


Not sure we should remove this. Keep in mind that there is no other try-catch outside of this. So somehow, just to sleep well we should make sure no exception caused by the agent can escape from this - e.g. on my machine currently die to the exception on request.Body.Position = 0; (discussed separately) would crash the whole app.

Either we deal with it here, or we make sure on the caller side that there is no case which could affect the application.

If there is no exception handler in a wrapper context (calling function/class etc) then obviously we need to keep the general exception handler block. I'll remove the comment.

👍 Sounds good! We plan to have a better solution for this long term (#208) but currently every exception caused by the agent will bubble up to the user code unless we manually try-catch.

gregkalapos · 2019-07-25T15:02:11Z

src/Elastic.Apm.AspNetCore/Extensions/RequestExtentions.cs

+
+				}
+			} catch (IOException ioException){
+				logger.Error()?.Log("IO Error reading request body , Error msg : {exception} , stack trace : {stacktrace} " , ioException.Message , ioException.StackTrace);


jfyi: we have an overload that logs the exception for you:

logger.Error()?.LogException(ioException, "IO Error reading request body");

The result is same to what you did here manually.

Same below.

I updated the code to use logger.Error()?.LogException.

src/Elastic.Apm.AspNetCore/Helpers/AgentConfigHelper.cs

1. Created a new Exception filter in the Elastic.Apm.AspNetCore project to be used instead of the Elastic.Apm one (to enable request body capturing). 2. Added the configuration option 'CaptureBodyContentTypes' that defines which request content types will be matched with the requests. The default value is common text content types : application/x-www-form-urlencoded* text/* application/json* application/xml* 3. Some refactoring and code-arranging. 4. Added some tests

katzdan · 2019-07-30T11:47:19Z

@gregkalapos , @SergeyKleyman - I'm done with the implementation, with 3 issues:

I'm still unable to re-create the case where reading the request body fails with the error Unexpected end of Stream, the content may have already been read by another component..
From what I read - 'EnableRewind' is available for .net standard 2.0 apps via the Microsoft.AspNetCore.Http.Extensions nuget package which is supposed to resolve this problem. I just wanted your guys' approval before adding a new package to the project.
When the user configures 'CaptureBody' to all/errors/transactions but there is no 'CaptureBodyContentTypes' property in the appsettings.json file - the Apm agent will report it in the logs ("Capture_Body is on but no content types are configured. Request bodies will not be captured.") and the agent will not extract the request body in any scenario because the list of allowed content-types is empty (and the agent matches the request content-type with the entries in the list). In this case - I'm asking if it makes more sense to use the default content types instead of an empty one. Your thoughts?
The checks for this commit are failing due to some code checkout error. How can I trigger the checks again (without committing)? All tests run fine on my machine.

katzdan · 2019-08-04T12:10:37Z

@SergeyKleyman , @gregkalapos ,
Have you see my latest comment?

SergeyKleyman · 2019-08-04T12:13:30Z

@katzdan Hi Dan. We have a few urgent things to finish for the latest release and we will get back to this feature ASAP.

gregkalapos · 2019-08-04T21:59:39Z

Hey @katzdan, sorry for the delay!

I can’t repro that problem either anymore.. I checked out your latest code and I don’t see that problem. Nevertheless I think it’d be nice to use EnableRewind - it’s fine to reference Microsoft.AspNetCore.Http.Extensions 👍. Please make sure to reference Version 2.0.0.
In such situations we usually look at what other Elastic APM Agents do, and if it is not something totally unnatural for .NET, then we tend to just adapt it. The Java agent seem to do exactly what you suggest - it has a default, and that is used when no CaptureBodyContentTypes is configured - the only thing I don’t see in the default list is multipart/form-data. I’m happy to hear thoughts on this: shouldn’t we also add that?
I think there are 2 reasons for not building the PR - 1) there are some conflicts and that’s why Jenkins did not build it. 2) our CI only automatically builds PRs from either devs from the Elastic org, or PRs that one of us approved. This is a security mechanism. I lost track on this.. was this built by the CI previously? I think it wasn’t. Could you please resolve the conflicts? I’ll then take care of running it in CI once that’s done.

I think we are almost done here. :)

katzdan · 2019-08-08T19:47:21Z

@gregkalapos , @SergeyKleyman -

I'll add the HttpExtensions lib (2.0), no problem.
In regard to multipart/form-data - it may contain binary data (uploaded files in a HTML form) so I would think it makes sense not to include it in this PR , stay in line with the Java implementation ad review the option to include it in the future.
I'll merge the master branch from the Elastic .net Apm repo into my work branch , commit in order to resolve the conflicts and then we'll see if the ci builds still fail.

gregkalapos · 2019-08-08T19:49:02Z

@gregkalapos , @SergeyKleyman -

I'll add the HttpExtensions lib (2.0), no problem.

In regard to multipart/form-data - it may contain binary data (uploaded files in a HTML form) so I would think it makes sense not to include it in this PR , stay in line with the Java implementation ad review the option to include it in the future.

I'll merge the master branch from the Elastic .net Apm repo into my work branch , commit in order to resolve the conflicts and then we'll see if the ci builds still fail.

All sounds good @katzdan 👍

1. Added a reference to 'Microsoft.AspNetcore.Http' ver 2.0.0 2. Added a call to 'EnableRewind' when reading the request body and reset the body stream position to 0 after that in case subsequent readings of the body. 3. Removed a commented line in AspNetCoreMiddlewareTests.cs

katzdan · 2019-08-09T15:27:07Z

@gregkalapos , @SergeyKleyman -
I added the call to 'EnableRewind' (it turns out that I needed to add a reference to 'Microsoft.AspNetCore.Http' 2.0.0 and not to the HttpExtensions lib). The tests pass and a manual check also passed. I resolved the conflicts yesterday so I guess you guys can continue with the merge (unless there anything additional you need from me).

gregkalapos · 2019-08-09T15:39:50Z

@gregkalapos , @SergeyKleyman -
I added the call to 'EnableRewind' (it turns out that I needed to add a reference to 'Microsoft.AspNetCore.Http' 2.0.0 and not to the HttpExtensions lib). The tests pass and a manual check also passed. I resolved the conflicts yesterday so I guess you guys can continue with the merge (unless there anything additional you need from me).

Thanks, great; will take a look at it soon!

skynet2 · 2019-08-12T07:32:41Z

hi all,
quick question, net core give us the ability to use middlewares for that purpose, maybe it will be easier to use it? for example here is the code which I wrote to have APM logging for both request + response https://gist.github.com/skynet2/b0c9b96f0c3a75175962d6831b403339

Thanks,
Stas

gregkalapos · 2019-08-12T09:55:46Z

hi all,
quick question, net core give us the ability to use middlewares for that purpose, maybe it will be easier to use it? for example here is the code which I wrote to have APM logging for both request + response https://gist.github.com/skynet2/b0c9b96f0c3a75175962d6831b403339

Thanks,
Stas

Hi @skynet2,

yes, that's exactly what's happening here. In this PR the reading of the request body happens in the ApmMiddleware here.

So I think we have the same approach here as in your code. Or do I miss something?

gregkalapos

I went through it again, this looks good to me, thank you very much @katzdan! 🚀

There are some minor code styling things - I started focusing more on those, because sometimes people run auto cleanup before they open a PR and all those tiny things are reformatted based on the .editorconfig causing lots of minor code style changes without any functional change and that makes PRs harder to review. Once there aren't too many PRs open I'll do a bigger cleanup (see #427) If you want I can quickly fix those, just let me know - otherwise just feel free to push it.

Other than that I think this is ready to merge. Thanks again!

src/Elastic.Apm/Config/ConfigConsts.cs

gregkalapos · 2019-08-12T10:13:22Z

src/Elastic.Apm/Config/AbstractConfigurationReader.cs

 			Ms,
 			S
 		}
+


nit: there are 2 new lines here.

src/Elastic.Apm/Config/AbstractConfigurationReader.cs

src/Elastic.Apm.AspNetCore/Helpers/ExceptionFilter.cs

src/Elastic.Apm.AspNetCore/ApmMiddleware.cs

codecov-io · 2019-08-12T10:44:59Z

Codecov Report

Merging #402 into master will decrease coverage by 0.34%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##           master     #402      +/-   ##
==========================================
- Coverage   79.36%   79.01%   -0.35%     
==========================================
  Files          80       84       +4     
  Lines        2777     2812      +35     
  Branches      517      509       -8     
==========================================
+ Hits         2204     2222      +18     
- Misses        385      394       +9     
- Partials      188      196       +8

Impacted Files	Coverage Δ
...astic.Apm/Config/EnvironmentConfigurationReader.cs	`94.73% <100%> (+0.61%)`	⬆️
src/Elastic.Apm/Config/ConfigConsts.cs	`100% <100%> (ø)`	⬆️
...tCore/Extensions/IConfigurationReaderExtentions.cs	`100% <100%> (ø)`
...lastic.Apm.AspNetCore/Extensions/ListExtensions.cs	`100% <100%> (ø)`
...Apm.AspNetCore/Config/MicrosoftExtensionsConfig.cs	`78.57% <100%> (+1.07%)`	⬆️
.../Elastic.Apm/Config/AbstractConfigurationReader.cs	`73.39% <59.25%> (-1.34%)`	⬇️
...tic.Apm.AspNetCore/Extensions/RequestExtentions.cs	`61.29% <61.29%> (ø)`
...Apm.AspNetCore/Extensions/TransactionExtentions.cs	`75% <75%> (ø)`
.../Elastic.Apm.AspNetCore/Helpers/ExceptionFilter.cs	`87.5% <87.5%> (ø)`
...stic.Apm.AspNetCore/Extensions/StringExtensions.cs	`87.5% <87.5%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 418fc24...d3384e3. Read the comment docs.

katzdan · 2019-08-14T07:13:58Z

@gregkalapos - I'll make the changes today or tomorrow.

Several small code clean up changes.

katzdan · 2019-08-15T08:07:49Z

@gregkalapos - I pushed the files updated in the code clean up. You can merge this into the Master branch unless there are additional changes to be implemented.

Remove some unused fields

gregkalapos · 2019-08-15T11:25:41Z

Merged! 🎉

Thanks for your work and your patience on this @katzdan. Welcome on the contributors list! 🚀

brezaie · 2019-08-19T11:29:50Z

Hi pals,
Adding the following configuration to the appsettings.json:

"ElasticApm": { "ServerUrls": "***", "ServiceName": "SomeService", "CaptureHeaders": true, "CaptureBody": "all", "LogLevel": "Info", "StackTraceLimit": -1 }

I received the following log message with does not show what the body request and response are:

gregkalapos · 2019-08-19T11:35:18Z

Hi @brezaie

I see you don’t configure CaptureBodyContentTypes (here is an example for that). I suspect that this is some content type that is not included in the default. Is it maybe multipart/form-data?

Also please note that this PR is only about capturing the request body - we don’t capture the response body.

brezaie · 2019-08-19T11:40:15Z

Hi @gregkalapos

Thanks for your response. It did work now.

Do you have any plans for adding the response body?

gregkalapos · 2019-08-19T11:44:04Z

Hi @gregkalapos

Thanks for your response. It did work now.

Do you have any plans for adding the response body?

Unfortunately we have no such plans currently. response body is not part of the API that the APM server expects from us. We have a field for it in request, bot not in response.

brezaie · 2019-08-21T04:29:34Z

Dear @gregkalapos

Thanks for your reply.

When adding the following configuration, surprisingly, I receive a null for the input of my POST method
"CaptureBodyContentTypes": "application/x-www-form-urlencoded*, text/*, application/json*, application/xml*",

Here is my POST method and the corresponding filter and configuration:

[HttpPost("send")]
public JsonResult Send([FromBody] BaseReportFilter<SendMessageFilter> filter)
{
	//Do something
	return new JsonResult("");
}


public class BaseReportFilter<T>
{
    public T ReportFilter { get; set; }
}

public class SendMessageFilter{
	public string SenderApplicationCode { get; set; }
	public string MediaType { get; set; }
	public string Body { get; set; }
	public List<string> Recipients { get; set; }
}

//appsettings.json:
"ElasticApm": {
    "ServerUrls": "http://localhost:8200",
    "ServiceName": "ServiceName",
    "CaptureHeaders": true,
    "CaptureBody": "all",
    "CaptureBodyContentTypes": "application/x-www-form-urlencoded*, text/*, application/json*, application/xml*",
    "LogLevel": "Info",
    "StackTraceLimit": -1
  }

Also, you can see my API call from Postman here:

gregkalapos · 2019-08-26T14:39:19Z

Dear @gregkalapos

Thanks for your reply.

When adding the following configuration, surprisingly, I receive a null for the input of my POST method
"CaptureBodyContentTypes": "application/x-www-form-urlencoded*, text/*, application/json*, application/xml*",

Here is my POST method and the corresponding filter and configuration:
[HttpPost("send")]
public JsonResult Send([FromBody] BaseReportFilter<SendMessageFilter> filter)
{
	//Do something
	return new JsonResult("");
}


public class BaseReportFilter<T>
{
    public T ReportFilter { get; set; }
}

public class SendMessageFilter{
	public string SenderApplicationCode { get; set; }
	public string MediaType { get; set; }
	public string Body { get; set; }
	public List<string> Recipients { get; set; }
}

//appsettings.json:
"ElasticApm": {
    "ServerUrls": "http://localhost:8200",
    "ServiceName": "ServiceName",
    "CaptureHeaders": true,
    "CaptureBody": "all",
    "CaptureBodyContentTypes": "application/x-www-form-urlencoded*, text/*, application/json*, application/xml*",
    "LogLevel": "Info",
    "StackTraceLimit": -1
  }
Also, you can see my API call from Postman here:

Thanks for reporting this @brezaie. This is not good - I also managed to reproduce this. I opened a separate bug for this: #460. If there is anything to add to it, feel free to comment there.

katzdan added 4 commits July 21, 2019 03:00

Capture request body in asp.net core

b967135

initial feature commit - Added config values + behavior in the Apm middle-ware class. Errors body extraction still pending.

Added the body default value

9393ef6

Minor fixes

57d2cf1

Had to make minor changes to the code due to some name changes.

Check if body > 2kb and truncate accordingly

84ca1a8

gregkalapos reviewed Jul 21, 2019

View reviewed changes

katzdan added 3 commits July 23, 2019 13:11

Added error logging in ApmMiddle

11ade4f

1. Added request body capturing in the ApmMiddle InvokeAsync method. 2. Added a test to verify this scenario. 3. Refactored some of the code that determines if the agent should capture the request body for better readability and maintainability.

Merge origin/master into the development branch

d23a5f1

gregkalapos self-assigned this Jul 24, 2019

gregkalapos reviewed Jul 25, 2019

View reviewed changes

gregkalapos mentioned this pull request Jul 25, 2019

Capture request body in ASP.NET Full Framework #379

Closed

SergeyKleyman mentioned this pull request Aug 7, 2019

When the request body is added for .net core? #435

Closed

Merging master into working branch to close the gap

920eb74

gregkalapos mentioned this pull request Aug 10, 2019

Capture body feature #439

Closed

gregkalapos approved these changes Aug 12, 2019

View reviewed changes

katzdan added 2 commits August 14, 2019 22:56

Code clean-up

1d6aacb

Several small code clean up changes.

Additional (missed) code clean up change

c008e75

Update src/Elastic.Apm/Config/ConfigConsts.cs

fcd5b20

Remove some unused fields

gregkalapos merged commit 4324052 into elastic:master Aug 15, 2019

gregkalapos mentioned this pull request Aug 15, 2019

Capture request body in ASP.NET Core #213

Closed

SergeyKleyman mentioned this pull request Aug 26, 2019

Document CAPTURE_BODY and CAPTURE_BODY_CONTENT_TYPES configuration options #452

Closed

gregkalapos mentioned this pull request Aug 26, 2019

ASP.NET Core request body capturing erases the body #460

Closed

Conversation

katzdan commented Jul 21, 2019 • edited by SergeyKleyman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katzdan commented Jul 21, 2019

Uh oh!

gregkalapos commented Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergeyKleyman commented Jul 21, 2019

Uh oh!

gregkalapos commented Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katzdan commented Jul 21, 2019

Uh oh!

gregkalapos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katzdan Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katzdan commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katzdan commented Jul 24, 2019

Uh oh!

SergeyKleyman commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergeyKleyman commented Jul 24, 2019

Uh oh!

katzdan commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregkalapos commented Jul 25, 2019

Uh oh!

gregkalapos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

katzdan commented Jul 21, 2019 •

edited by SergeyKleyman

Loading

gregkalapos commented Jul 21, 2019 •

edited

Loading

gregkalapos commented Jul 21, 2019 •

edited

Loading

katzdan Jul 23, 2019 •

edited

Loading

katzdan commented Jul 22, 2019 •

edited

Loading

SergeyKleyman commented Jul 24, 2019 •

edited

Loading

katzdan commented Jul 24, 2019 •

edited

Loading

gregkalapos Jul 25, 2019 •

edited

Loading

codecov-io commented Aug 12, 2019 •

edited

Loading