Support getting CAPTURE_BODY and TRANSACTION_MAX_SPANS options from central config by SergeyKleyman · Pull Request #577 · elastic/apm-agent-dotnet

SergeyKleyman · 2019-10-29T14:53:44Z

Closes #534

…entral config

…ral_config_part_2

bmorelli25

Nice!

docs/configuration.asciidoc

Co-Authored-By: Brandon Morelli <bmorelli25@gmail.com>

gregkalapos

I think overall this is fine - sorry that it took so long for me to go through this.

If I may add some suggestions:

If possible please add some comments in the code - that really helps others to understand it (I added below some specific places where it'd help). When you write it, it's totally obvious, I know - but 1-2 lines of comments here and there can make a huge difference for the reader.
I think we still have lots of unrelated things in this PR - for now it's fine, I'd not change those, but in the future, especially with these bigger PRs, could we please just focus on what the PR title says? That'd make reviewing easier.

gregkalapos · 2019-10-29T21:34:05Z

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

 namespace Elastic.Apm.AspNetCore.Extensions
 {
-	public static class TransactionExtensions
+	internal static class TransactionExtensions


Hmm.. this is technically a breaking change. Probably no one uses it, because this should not be public and I 100% agree with that. I already almost made the same change in one of my previous PRs.

Still I think it'd be best to leave it public and once we release 2.0 make it internal - and go through the code base and fix if there are other similar problems (I think there are).

Unfortunately this was a mistake, but I think it'd be better to fix this on a major release bump.

I agree with your assessment:

Probably no one uses it

So is it worth spending any more time on it?

So is it worth spending any more time on it?

Yes, because it's just about changing internal back to public and we have 1 less breaking change. That can be done very quickly.

Unfortunately it's not as simple as changing internal back to public because the signature of CollectRequestInfo changed.

Oh, yeah I see. As said here, I'd left it as it is. I know, the IConfigurationReader went away, but technically we could still pass that as a parameter.

Anyway, ok, let's make it internal and hope no one uses it. But could we try to avoid these things in the future please?

Definitely.

gregkalapos · 2019-10-29T21:56:10Z

src/Elastic.Apm/Model/Error.cs

 			ParentId = parentId;
 			Transaction = new TransactionData(transaction.IsSampled, transaction.Type);

+			if (transaction.IsSampled) Context = transaction.Context;


I see you move capturing the context from QueueError to the Error .ctor. Is that related to the new configs coming from central config? I think it's not and I'd leave out such changes from these PRs.

I think we discussed on slack that transaction's context should be separated from the error's context. So instead of sharing it, we should capture those differently, in case one of those changes.

Probably similar to other agents (or at least Python) we should also capture Error.Context always - also in case the transaction is not sampled.

But I feel none of these are related to this PR, so I'd prefer everything related to this outside of the PR - unless I overlooked something and it's directly related to fetching the setting values from central config.

gregkalapos · 2019-11-13T17:47:06Z

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

-		public static void CollectRequestInfo(this ITransaction transaction, HttpContext httpContext, IConfigurationReader configurationReader,
-			IApmLogger logger
-		)
+		internal static void CollectRequestBody(this Transaction transaction, bool isForError, HttpRequest request, IApmLogger logger)


I think the only thing related to adding CAPTURE_BODY to central config is that the IConfigurationReader is changed to transaction.ConfigSnapshot, right? Would it be possible that we do other changes (like making this method to capture the body both for errors and not errors, restructuring the code, etc.) in separate PRs in the future? It would make reviews easier - the smaller the PR is the easier it is to review.

gregkalapos · 2019-11-13T17:51:49Z

src/Elastic.Apm/Config/AbstractConfigurationReader.cs


 				if (nameInConfig == adaptedServiceName)
-					_logger?.Warning()?.Log("Service name provided in configuration is {ServiceName}", nameInConfig);
+					_logger?.Debug()?.Log("Service name provided in configuration is {ServiceName}", nameInConfig);


This is no big deal, but this is I think again unrelated to the title of the PR. I usually switch to master, do this change, create a PR with a new branch and merge these separately - with git this really takes no time. That helps keeping these big PRs smaller.

gregkalapos · 2019-11-13T18:07:14Z

src/Elastic.Apm/Config/AbstractConfigurationReader.cs

 		}

-		protected List<string> ParseCaptureBodyContentTypes(ConfigurationKeyValue kv, string captureBody)
+		protected List<string> ParseCaptureBodyContentTypes(ConfigurationKeyValue kv)


Sooo..what happens here is that since CAPTURE_BODY is now live changeable, we can't take that into consideration when we read CaptureBodyContentTypes, right?

That is correct.

gregkalapos · 2019-11-13T18:16:47Z

test/Elastic.Apm.AspNetCore.Tests/BodyCapturingTests.cs

+
+		[Theory]
+		[MemberData(nameof(OptionsChangedAfterStartTestVariants))]
+		public async Task Options_changed_after_start(int startCfgVariantIndex, OptionsTestVariant startCfgVariant)


It'd be really nice to add some comments to this one. To be honest, I already started going through this PR before EAH and this was the point where I gave up - some comments would really help here.

gregkalapos · 2019-11-13T18:28:04Z

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

+		}
+
+		private static bool IsCaptureRequestBodyEnabled(Transaction transaction, bool isForError) =>
+			transaction.ConfigSnapshot.CaptureBody.Equals(ConfigConsts.SupportedValues.CaptureBodyAll)


At least to me IConfigurationReaderExtentions with ShouldExtractRequestBodyOnError and ShouldExtractRequestBodyOnTransactions was easier to follow than this method. - no action needed on this, just saying.

gregkalapos · 2019-11-13T18:33:38Z

src/Elastic.Apm/Model/Transaction.cs

 		public Context Context => _context.Value;

+		[JsonIgnore]
+		internal IConfigSnapshot ConfigSnapshot { get; }


So, if I understand correctly, the plan is that this is the ConfigSnapshot instance that we pass to every other component like Span and also this instance is used by things like TransactionExtensions, right? If I'm right, then this seems to be a fairly important property and I'd comment it. If I'm wrong, then I'd still comment it, because after looking at the code long enough I got the wrong impression.

gregkalapos · 2019-11-13T18:37:41Z

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

-			{
-				var contentType = new ContentType(httpContext.Request.ContentType);
+			// Is request body already captured?
+			if (transaction.IsContextCreated


Why does transaction.IsContextCreated matter here? 🤔 Is it because transaction.Context.Request would create one? If so, is there any case when only the Body is filled? I don't think so... so if we are here, I think we already have a context anyway, or not?

I didn't want to assume too much about the context in which this method is called - the purpose was to avoid creating empty Context. I've added a comment to the code.

Ok, sounds good to me.

gregkalapos · 2019-11-13T18:42:40Z

src/Elastic.Apm.AspNetCore/DiagnosticListener/AspNetCoreDiagnosticListener.cs

 			var exception = kv.Value.GetType().GetTypeInfo().GetDeclaredProperty("exception").GetValue(kv.Value) as Exception;

-			var transaction = _agent.Tracer.CurrentTransaction;
+			var transaction = (Transaction)_agent.Tracer.CurrentTransaction;


Hmm.. this cast is a little bit unfortunate - with this it MUST be a Transaction, otherwise it will throw an exception - so e.g. we can't just replace to NoOpTransaction later if needed.

I guess the root cause of the challenge here is that we don't dependency inject the configuration anymore, instead it comes through the Transaction. I'm not sure this is good - could we maybe find another way to flow the ConfigSnapshot?

It's not like it's the only place where we assume that ITransaction is actually Transaction - for example

apm-agent-dotnet/src/Elastic.Apm/Report/PayloadSenderV2.cs

Line 219 in a73b10f

case Transaction _:

)

If/when we need to have other implementations of ITransaction we will revisit it. For example we can introduce TransactionBase and move ConfigSnapshot there.

I'd consider a pattern matching based on types very different from a hard cast.

But I think we are good here. In #608 I moved this part out from here, so probably we can also get rid of the cast once we merge that.

Context: In #608 I realized that capturing request body here is not a very good thing, because this is a sync method and reading the body synchronously can cause thread starvation. So I'd say let's leave it as is, and in #608 this will evolve.

gregkalapos

Discussed on slack.

I hoped to move some of the unrelated changes out from here, or at least some reaction to those here.

Overall no blocker on this PR, but if possible please try to keep these smaller in the future, otherwise it's very hard to review.

Reviewing a couple of hundred lines can be done in a couple of minutes, but having a 1000+ lines PR with 31 files changing, with lots of moving parts takes significantly longer. Adding some comments and making it as small as possible helps the reviewer a lot.

SergeyKleyman added 2 commits October 28, 2019 08:05

Add CentralConfigTests.TransactionSample_test

e27fefa

Support getting CAPTURE_BODY and TRANSACTION_MAX_SPANS options from c…

0a6fc2b

…entral config

SergeyKleyman requested a review from gregkalapos October 29, 2019 14:53

SergeyKleyman self-assigned this Oct 29, 2019

SergeyKleyman added 7 commits October 29, 2019 16:55

Add CentralConfigTests.TransactionSample_test

b431d21

Support getting CAPTURE_BODY and TRANSACTION_MAX_SPANS options from c…

cc20293

…entral config

Merge remote-tracking branch 'origin/Central_config_part_2' into Cent…

b829467

…ral_config_part_2

Merge branch 'master' into Central_config_part_2

6585677

Fix performance overhead of capturing body twice on error

5abcb77

Fix failing AspNetFullFramework.Tests

08113e5

Updates docs

3f082ee

SergeyKleyman added the docs label Oct 31, 2019

SergeyKleyman requested a review from bmorelli25 October 31, 2019 06:46

bmorelli25 approved these changes Oct 31, 2019

View reviewed changes

docs/configuration.asciidoc Show resolved Hide resolved

docs/configuration.asciidoc Outdated Show resolved Hide resolved

docs/configuration.asciidoc Outdated Show resolved Hide resolved

docs/configuration.asciidoc Show resolved Hide resolved

SergeyKleyman and others added 4 commits November 1, 2019 10:56

Update docs/configuration.asciidoc

c3ffd11

Co-Authored-By: Brandon Morelli <bmorelli25@gmail.com>

Update docs/configuration.asciidoc

9d5c35e

Co-Authored-By: Brandon Morelli <bmorelli25@gmail.com>

Merge branch 'master' into Central_config_part_2

955cc12

Minor "All options summary" section update

d51d984

gregkalapos reviewed Nov 13, 2019

View reviewed changes

This was referenced Nov 15, 2019

Documentation for CaptureBodyContentTypes is not fit with implementation #603

Closed

Request body capturing - use EnableBuffering instead of EnableRewind and make it async #608

Merged

SergeyKleyman added 3 commits November 19, 2019 12:32

Add comments to Options_changed_after_start test

bc3106d

Add comment to Transaction.ConfigSnapshot

62cb907

Add comment to CollectRequestBody

c1c999a

gregkalapos self-requested a review November 19, 2019 14:49

gregkalapos approved these changes Nov 19, 2019

View reviewed changes

SergeyKleyman merged commit 9cc92fa into elastic:master Nov 19, 2019

SergeyKleyman deleted the Central_config_part_2 branch November 19, 2019 15:11

Conversation

SergeyKleyman commented Oct 29, 2019 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmorelli25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gregkalapos left a comment

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!

Choose a reason for hiding this comment

Uh oh!

gregkalapos Nov 13, 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!

Choose a reason for hiding this comment

Uh oh!

gregkalapos Nov 13, 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!

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!

gregkalapos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SergeyKleyman commented Oct 29, 2019 •

edited by zube bot

Loading

gregkalapos Nov 13, 2019 •

edited

Loading

gregkalapos Nov 13, 2019 •

edited

Loading