String truncation by gregkalapos · Pull Request #135 · elastic/apm-agent-dotnet

gregkalapos · 2019-02-28T22:40:00Z

Solves #129.

This is very Json.NET specific. Maybe we move to another json serializer later, but still I think it's nice to have this now, so I'd say: let's solve it for the current serializer 🚀

After this change:

by default we trim every string property
by default we also trim the values inside Dictionary<string, string>
in case we don't want to trim something we can mark it with [NoTruncationInJsonNet]

added tests.

Plus some cleanup

gregkalapos · 2019-02-28T22:41:19Z

src/Elastic.Apm/Model/Payload/Span.cs

-			get => _name;
-			set
-			{
-				if (value.Length > Consts.PropertyMaxLength)


We don't truncate here anymore. This happens before serialization.

gregkalapos · 2019-02-28T22:43:33Z

test/Elastic.Apm.Tests/BasicAgentTests.cs

-			agent.Tracer.CaptureTransaction("TestTransaction", "Test", (t) => { t.CaptureSpan(spanName.ToString(), "test", () => { }); });
-
-			Assert.NotNull(payloadSender.FirstSpan);
-			Assert.Equal(1024, payloadSender.FirstSpan.Name.Length);


This isn't valid anymore. We can think about testing more in PayloadSender, but atm if we go though MockPayloadSender the trimming logic is not triggered. With Intake V2 there'll be a new PayloadSender, so I'd rather not focus on this now... we have good coverage by the other new tests I added.

++ MockPayloadSender is only used to assert messages get send not necessarily how.

More isolated serialization tests would be helpful

skynet2 · 2019-02-28T23:30:23Z

HI @gregkalapos ,
Correct me if I'm wrong but your code will truncate all string and all dictionaries.
According to json schema only some fields should be truncated, maybe it`s better to create attribute Truncate and truncate only that strings which are marked with that attribute?

gregkalapos · 2019-03-01T15:54:23Z

HI @gregkalapos ,
Correct me if I'm wrong but your code will truncate all string and all dictionaries.
According to json schema only some fields should be truncated, maybe it`s better to create attribute Truncate and truncate only that strings which are marked with that attribute?

By default we truncate everything. I basically have an inverse logic here: by default we truncate, and if we don't want to truncate something we add [NoTruncationInJsonNet] to it. I do this, because at first glance I think we have more fields with a maxLength limit than fields without limit and in case we don't truncate the server would reject the whole payload, so it's better to be on the safe side.

I added the attribute to (I think) all fields where it's needed. It'd be nice to see #134 merged (since that moves some types that are effected here), and then I'd make Span.Context things also public and then I'll go through all fields again to make sure we don't miss anything we should not truncate.

SergeyKleyman

LGTM

Mpdreamz · 2019-03-04T13:13:53Z

src/Elastic.Apm/Model/Payload/Stacktrace.cs

 {
 	internal class Stacktrace
 	{
+		[NoTruncationInJsonNet]


Would it be cleaner to declare the reverse? [Truncate(int chars)] instead?

I just spotted @skynet2 comment. I tend to agree, a default truncate feels implicit rather than explicit.

Ok, since my logic seems to be strange for basically everyone, we go with the reverse.

Just for reference: the main reason for trimming by default was this:
If we trim by default and we forget to add [DontTruncate] -> We trim the string, but the first 1024 chars and everything else is visible.
If we don't trim and forget to add [Truncate] -> The server rejects the request (the whole transaction or span), nothing is visible.

Of course we should not forget to add attributes in the first place :)

Anyway, I accept that trimming by default would be strange. Reversed the logic.

Mpdreamz · 2019-03-04T13:25:04Z

src/Elastic.Apm/Report/Serialization/StringTruncationValueResolver.cs

+	/// Automatically applies <see cref="StringValueProvider"/> to <see cref="String"/> properties, and <see cref="StringDictionaryValueProvider"/>
+	/// to <see cref="Dictionary{String, String}"/> unless they are marked with <see cref="NoTruncationInJsonNetAttribute"/>.
+	/// </summary>
+	internal class StringTruncationValueResolver : CamelCasePropertyNamesContractResolver


Could potentially just be two JsonConverter e.g TrimmedStringJsonConverter and TagsJsonConverter (for the Dictionary<string, string> case handled here as well.

This would eschew the custom contract resolver and value providers.

👍 Agreed, it makes things easier. Added the two converters, removed the contract resolver and the value provider.

Removed NoTruncationInJsonNetAttribute, by default we do not trancate strings.

gregkalapos · 2019-03-05T00:45:58Z

@Mpdreamz thanks for the feedback!

I pushed another version.

I created the 2 converters, and I simply attach those to the properties that we want to trim. I think this is the easiest and simplest solution.

Another option would be to activate those globally through JsonSerializerSettings, but in that case we need additional attributes to mark things for trimming. I think the current solution is better, because this way we don't have to check for attributes within the converter. Once the converter fires we can trim, since we only add those to properties that we wanna trim.

....will rebase this PR soon....

Mpdreamz

I left a super small suggestion, nothing worth blocking merging over 😄

Thanks for the rewrite to converters, this will also help when we later on move to a different serializer!

sample/ApiSamples/Program.cs

Co-Authored-By: gregkalapos <gergo@kalapos.net>

…into elastic-master

Removed SpanNameLengthTest. since it's coverd in SerializationTests

codecov-io · 2019-03-07T11:10:31Z

Codecov Report

Merging #135 into master will increase coverage by 3.43%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   80.03%   83.46%   +3.43%     
==========================================
  Files          40       40              
  Lines        1292     1173     -119     
  Branches      207      187      -20     
==========================================
- Hits         1034      979      -55     
+ Misses        188      122      -66     
- Partials       70       72       +2

Impacted Files	Coverage Δ
src/Elastic.Apm/Model/Payload/Stacktrace.cs	`100% <ø> (ø)`	⬆️
src/Elastic.Apm/Model/Payload/Error.cs	`80% <ø> (-3.34%)`	⬇️
src/Elastic.Apm/Api/Context.cs	`100% <ø> (ø)`	⬆️
src/Elastic.Apm/Model/Payload/Transaction.cs	`97.38% <ø> (+11.66%)`	⬆️
src/Elastic.Apm/Consts.cs	`100% <ø> (ø)`	⬆️
src/Elastic.Apm/Report/PayloadSender.cs	`65.15% <ø> (+6.26%)`	⬆️
src/Elastic.Apm/Helpers/StringExtensions.cs	`100% <100%> (+100%)`	⬆️
src/Elastic.Apm/Model/Payload/Span.cs	`89.47% <100%> (+1.41%)`	⬆️
...Report/Serialization/TrimmedStringJsonConverter.cs	`50% <50%> (+50%)`	⬆️
...stic.Apm/Report/Serialization/TagsJsonConverter.cs	`90% <90%> (+90%)`	⬆️
... and 6 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 d40b0a4...749efff. Read the comment docs.

gregkalapos added 4 commits February 28, 2019 17:27

Add string trimming - WIP #129

e068f80

Add StringTruncationTest

8e0b526

Plus some cleanup

Add NoTruncateAttributeTest

b1f3688

Handling truncation of values in Dictionary<string,string> #129

71da5aa

gregkalapos requested a review from SergeyKleyman February 28, 2019 22:40

gregkalapos mentioned this pull request Feb 28, 2019

Truncate strings #129

Closed

gregkalapos commented Feb 28, 2019

View reviewed changes

Add NoTruncationInJsonNet to strings that shouldn't be truncated

4e0c884

gregkalapos self-assigned this Mar 1, 2019

SergeyKleyman approved these changes Mar 4, 2019

View reviewed changes

Mpdreamz reviewed Mar 4, 2019

View reviewed changes

Mpdreamz requested changes Mar 4, 2019

View reviewed changes

Add TrimmedStringJsonConverter and TagsJsonConverter

8e4c434

Removed NoTruncationInJsonNetAttribute, by default we do not trancate strings.

Mpdreamz approved these changes Mar 7, 2019

View reviewed changes

sample/ApiSamples/Program.cs Outdated Show resolved Hide resolved

Mpdreamz and others added 4 commits March 7, 2019 11:25

Update sample/ApiSamples/Program.cs

9052446

Co-Authored-By: gregkalapos <gergo@kalapos.net>

Merge branch 'master' of https://github.com/elastic/apm-agent-dotnet …

fdadb23

…into elastic-master

Merge branch 'master' into StringTruncation

9b85050

Update test/Elastic.Apm.Tests/BasicAgentTests.cs

749efff

Removed SpanNameLengthTest. since it's coverd in SerializationTests

gregkalapos added 2 commits March 7, 2019 12:11

Update sample/ApiSamples/Program.cs

0f46637

Update src/Elastic.Apm/Elastic.Apm.csproj

19ff54f

gregkalapos merged commit 58e1302 into elastic:master Mar 7, 2019

Conversation

gregkalapos commented Feb 28, 2019

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!

skynet2 commented Feb 28, 2019

Uh oh!

gregkalapos commented Mar 1, 2019

Uh oh!

SergeyKleyman 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!

gregkalapos commented Mar 5, 2019

Uh oh!

Mpdreamz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Mar 7, 2019 •

edited

Loading