Skip to content

Expose Transaction.Context.Request and Transaction.Context.Response#134

Merged
gregkalapos merged 16 commits intoelastic:masterfrom
gregkalapos:ExposeContextV2
Mar 4, 2019
Merged

Expose Transaction.Context.Request and Transaction.Context.Response#134
gregkalapos merged 16 commits intoelastic:masterfrom
gregkalapos:ExposeContextV2

Conversation

@gregkalapos
Copy link
Copy Markdown
Contributor

@gregkalapos gregkalapos commented Feb 27, 2019

Potential solution to #124 - 2. Version, follow-up from #130

Types that we make public moved from Elastic.Apm.Model.Payload to Elastic.Apm.Api.

This makes both Context.Request and Context.Response public.

I also made Request, Response, Url, and Socket to a struct. Update: reverted, we use class.

With this dangerous code like this won't compile:

transaction.Context.Request.Method = "GET";

And code like this will work and won't throw, even if Request is not initialized:

var method = t.Context.Request.Method;

This'll also work and not throw:

var method = t.Context.Request.Url.Full;

I'll follow up on this, maybe we'll need to adjust our serialization to this, but I think it's worth it.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 27, 2019

Codecov Report

Merging #134 into master will increase coverage by 0.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   82.69%   83.05%   +0.36%     
==========================================
  Files          36       37       +1     
  Lines        1144     1151       +7     
  Branches      183      184       +1     
==========================================
+ Hits          946      956      +10     
  Misses        125      125              
+ Partials       73       70       -3
Impacted Files Coverage Δ
src/Elastic.Apm/Api/Context.cs 100% <100%> (ø)
src/Elastic.Apm/Api/Response.cs 100% <100%> (ø)
src/Elastic.Apm/Api/Request.cs 100% <100%> (ø)
src/Elastic.Apm.AspNetCore/ApmMiddleware.cs 83.33% <100%> (-0.31%) ⬇️
...astic.Apm/Config/EnvironmentConfigurationReader.cs 100% <0%> (ø)
src/Elastic.Apm/Logging/ConsoleLogger.cs 72.22% <0%> (+8.33%) ⬆️

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 0b7b8c9...c3d4ece. Read the comment docs.

@gregkalapos gregkalapos self-assigned this Feb 27, 2019
@gregkalapos
Copy link
Copy Markdown
Contributor Author

Reverted struct after discussing it.

Reason: This value-behaviour and forcing users to set everything at once would be strange in C#.

😢

Assert.True(payloadSender.FirstTransaction.Context.Response.Finished);
Assert.Equal(200, payloadSender.FirstTransaction.Context.Response.StatusCode);
}

Copy link
Copy Markdown
Contributor

@SergeyKleyman SergeyKleyman Mar 4, 2019

Choose a reason for hiding this comment

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

Maybe it's worth adding a test that sets a property of Request/Response object after construction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 Added a test doing that.

Copy link
Copy Markdown
Contributor

@SergeyKleyman SergeyKleyman left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants