Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Mar 4, 2019
Merged

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

merged 16 commits into from
Mar 4, 2019

Conversation

gregkalapos
Copy link
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

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