Feature/add custom #118

skynet2 · 2019-02-18T20:55:35Z

Add Custom fields. Add the possibility to specify request_body

=============
p.s i need some help from you guys. There is a possible problem that Request and Response will be null on Context object when you create transaction manually.
For now, i can see initialization for Request and Response only in internals libs such as Elastic.Apm.AspNetCore. The issue is that we need to have some mechanism of initialization for that objects outside of internal namespaces, for example for some custom rpc handlers or protocols. Any suggestions around that? We can pass some parameter to StartTransactionInternal, or pass some initial context object with request\response object or make that fields Lazy or smth like that.

Thanks,
Stas

gregkalapos

Hey @skynet2. Thank you very much for the PR!

the problem with adding Custom is that there is an ongoing discussion that it’ll be maybe deprecated: elastic/apm#7. (No final decision atm)

The plan is to use Tags instead. I know, there are some differences, since you can’t just put anything into tags, but on the other hand putting non-serialisable things into the context would cause troubles anyway. So I’d like to be on the safe side and not add something that we may need to remove from the public API soon.

Regarding your problem and the initialisation of Request and Response: We can definitely discuss this. The strategy was to only make things public in the preview release that are 100% remain public in the future - in order to avoid breaking changes.

But as we move forward, we’d like to extend the API and make more things public.

I think your approach with introducing an IRequest interface and make that public on ITransaction and then just returning the _context.Value?.Request in Transaction is a nice solution. I'd like to hide the Context itself, which is the case here, so that's fine. We can extend this to cover Response and add additional fields to the public interface. If you elaborate on your scenario, and you tell me which fields you need to set (I suspect it's things like StatusCode) we can discuss it.

So, in sum: we should revert the Custom part, sorry about that, but to avoid troubles later I think that’s the best option for now.

The IRequest part is fine, we can merge that. Usually we cover things with tests, if you are interested in writing those, just take a look at the tests/Elastic.Apm.Tests project and make sure that we cover the new fields and new interface in at least one test. Otherwise I’ll do it myself ;).

src/Elastic.Apm/Api/ISpan.cs

src/Elastic.Apm/Api/ITransaction.cs

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

skynet2 · 2019-02-21T16:41:31Z

Hi @gregkalapos ,
Thanks for the link to apm discussion, it`s sad about depreciation for custom, basically, the reason why i decided to use custom is that it can save big objects, for example, requests and response.

Basically, for the request, there is another way, as I added Request.RequestBody property which doesn`t have a limit on the length, but for the response, there is no such property.

About IRequest

Are there any specific fields you want to have on that interface or i can take all fields from the Request?
We need to decide about initialization for Request and Response, currently they are null by default, we have three ways: 1. make it get;set 2. initialize empty object on CreateTransactionInternal 3. Lazy
What do you think?

gregkalapos · 2019-02-21T18:54:10Z

I see, thanks for the clarification. Yes, I see the problem - and Tags are indeed limited on length.

Btw. what exactly do you want to store in the Custom? Is it both the request body and the response body? There was an issue opened for that couple of hours ago: elastic/apm#52. Would that help you?

To IRequest:

We can add all the fields, if we decide to make this public, there is no reason to keep any of those private. There are some properties which are other types, for those I’d add interfaces similarly.
Since these are bigger types and in some cases they won’t be needed I’d just wrap them into Lazy<T> (so option 3) - similarly to Tags.

skynet2 · 2019-02-23T13:49:28Z

Hello @gregkalapos ,
Thanks for the review. Currently, Im storing response json object in custom, for one of the services we need to store that data. Yep it would help, currently there are 3 issues in which im interested elastic/apm#52 elastic/apm#53 elastic/apm#7
So yep, while there is no final decision i`ll remove Custom from current pull request, and continue using in my own branch :).

Going to refactor Request\Response to Lazy.
Probably there is no reason to have IRequest which will have only 1 implementation and will not change, because the schema for apm server is fixed and there is no way to set an external object to that? what do you think @gregkalapos

In order to extract IReqest i will need to create also IUrl and ISocket... and make them also lazy. Maybe we can expose them as public, or you want to have them exactly as interface?
p.s
Not very familar with commit workflow for GitHub, we are using bitbucket in the company.
When you request changes, i do that changes, should i click on "resolve issue" or it should be clicked by you?

This reverts commit 84bcf95.

gregkalapos

Thank you for pushing the changes @skynet2!

Especially thanks for implemented trimming of strings, thats a very useful improvement... it was on our TODO list. 👍

Regarding custom: thanks for removing it, now I understand your use-case and I'll try to make sure we have something that covers your use-case. As you can see we have some contrary issues... let's see what the decision is and we'll implement something accordingly in the .NET agent.

Regarding IRequest and having only 1 implementation: what you say is totally right, nevertheless I'd still introduce interfaces. This would be the public API and the schema to the APM Server can change... we have changes all the time. Having only 1 class would make it very hard to move things forward and deprecate things if it's needed. We have this challenge with basically every class under Model/Payload. To give you an example: we consider renaming Tags to Labels, because we want to confirm to the elastic common schema. By having an interface we can keep Tags and add a new Labels field and mark Tags deprecated but and simply redirect the values to Labels. And long term in let's say 2-3 versions later we can remove the field. This is way better for existing users than to break something immediately. (Side note: as long as the .NET agent is not GA, we are ok with breaking things, but after GA this will change). Tags is just an example, and this can happen all the time. Usually I'm not a big fan of interfaces with 1 implementation, but the public API is something where we definitely have to be on the safe side and interfaces give us more flexibility. Hope this makes sense.

So to your comment:

In order to extract IReqest i will need to create also IUrl and ISocket... and make them also lazy. Maybe we can expose them as public, or you want to have them exactly as interface?

If we want to make them public I'd add interfaces to them and expose them through those interfaces.
Btw. the current change set is fine to merge, so if you don't need Url or Socket I'm ok with having those only as internal classes. In case you need those feel free to add an interface and expose them that way.

Not very familar with commit workflow for GitHub, we are using bitbucket in the company.
When you request changes, i do that changes, should i click on "resolve issue" or it should be clicked by you?

If you feel we agree on it and it's implemented feel free to push "resolve" - I personally also add a last comment with "done" or something similar as you did... with that other people can easily spot that it reached an agreement and it was implemented.

Lastly one small thing: we should have tests that cover 'User' and 'Body'. Before I merge I'll add tests for that.

Thank you for your help! I really appreciate it! 🚀

src/Elastic.Apm/Api/IRequest.cs

src/Elastic.Apm/Helpers/StringExtensions.cs

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

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

src/Elastic.Apm/Elastic.Apm.csproj

gregkalapos

@skynet2 tests in the Elastic.Apm.AspNetCore.Tests are failing because the lazy initialisations in Context.cs don't work. I added a comment on how to fix it.

Btw. we have standard XUnit tests... you can start those directly from Rider.

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

skynet2 · 2019-02-24T14:20:47Z

Hi @gregkalapos ,

Thanks, my fail, fixed.

Thanks,
Stas

gregkalapos

Great, thank you very much @skynet2!!

codecov-io · 2019-02-24T14:44:54Z

Codecov Report

Merging #118 into master will increase coverage by 1.3%.
The diff coverage is 69.23%.

@@            Coverage Diff            @@
##           master     #118     +/-   ##
=========================================
+ Coverage   84.09%   85.39%   +1.3%     
=========================================
  Files          35       37      +2     
  Lines        1012     1123    +111     
  Branches      130      147     +17     
=========================================
+ Hits          851      959    +108     
- Misses        115      116      +1     
- Partials       46       48      +2

Impacted Files	Coverage Δ
src/Elastic.Apm/Consts.cs	`100% <100%> (ø)`	⬆️
src/Elastic.Apm/Model/Payload/Span.cs	`92.42% <100%> (+1.04%)`	⬆️
src/Elastic.Apm/Report/PayloadSender.cs	`66.66% <20%> (+1.87%)`	⬆️
src/Elastic.Apm/Model/Payload/Request.cs	`92% <0%> (-8%)`	⬇️
src/Elastic.Apm/Model/Payload/Context.cs	`100% <0%> (ø)`	⬆️
src/Elastic.Apm/Model/Payload/User.cs	`0% <0%> (ø)`
src/Elastic.Apm/Helpers/StringExtensions.cs	`100% <0%> (ø)`
src/Elastic.Apm/Model/Payload/Transaction.cs	`98.08% <0%> (+0.7%)`	⬆️
... and 1 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 cbcff34...d5d58a4. Read the comment docs.

skynet2 · 2019-02-24T14:57:30Z

Hi @gregkalapos , please don`t merge yet, looks like there are some cases which are not covered by tests.

Currently, i`m getting that issue
Error Failed sending transaction. data validation error: Problem validating JSON document against schema: I[#] S[#] doesn't validate with "error#"
I[#/errors/0] S[#/properties/errors/items/allOf/0] allOf failed
I[#/errors/0/context/request/method] S[#/properties/errors/items/allOf/0/properties/context/properties/request/properties/method/type] expected string, but got null
Error Failed sending transaction. data validation error: Problem validating JSON document against schema: I[#] S[#] doesn't validate with "transaction#"
I[#/transactions/0] S[#/properties/transactions/items/allOf/0] allOf failed
I[#/transactions/0/context/request/method] S[#/properties/transactions/items/allOf/0/properties/context/properties/request/properties/method/type] expected string, but got null
Error Failed sending transaction. data validation error: Problem validating JSON document against schema: I[#] S[#] doesn't validate with "transaction#"
I[#/transactions/0] S[#/properties/transactions/items/allOf/0] allOf failed
I[#/transactions/0/context/request/method] S[#/properties/transactions/items/allOf/0/properties/context/properties/request/properties/method/type] expected string, but got null
Error Failed sending transaction. data validation error: Problem validating JSON document against schema: I[#] S[#] doesn't validate with "transaction#"
I[#/transactions/0] S[#/properties/transactions/items/allOf/0] allOf failed
I[#/transactions/0/context/request/url] S[#/properties/transactions/items/allOf/0/properties/context/properties/request/properties/url/type] expected object, but got null

gregkalapos · 2019-02-24T15:10:12Z

Och, good catch, I see!

The problem with Lazy is that once we do let's say this:

Agent.Tracer.CaptureTransaction("TestTransaction", "TestType",
t =>
{
t.Request.HttpVersion = "2";
}

Then we initialized Context.Request, but we don't make sure that all required fields are filled.

Hmm... this is an interesting problem. 🤔

skynet2 · 2019-02-24T15:34:50Z

I see two ways how we can solve that :

We can remove Lazy and make it get;set properties
We can create a custom JSON resolver for json.net which will ignore uninitialized lazy objects

What do you think @gregkalapos

skynet2 · 2019-02-24T16:03:42Z

The second way will not work, as it`s auto property.
The first way will require exposing to public Request and Response classes.

gregkalapos · 2019-02-24T16:08:49Z

Yes, and the problem with the 2. approach is that it can be partially initialized. E.g. in Request Method and Url are required. If we set e.g. Url, but leave Method empty then it's still not ok.

The "not so nice" thing about the 1. approach is that if those are properties, then if someone does not initialize it then there could be nullref exceptions.

I'm thinking about this... I'll try to come up with something soon :) Also feel free to let me know if you have something in mind.

gregkalapos · 2019-02-24T16:31:17Z

What about utilizing ValueTuples?

Instead of having an IRequest property on ITransaction we could introduce properties that expose all Request related fields at once:

public interface ITransaction
{
   (string httpVersion, string method, object body, string fullUrl,
      string rawUrl, string hostName, string protocol) Request { get; set; }
 //...
}

This way the user code either sets everything we need for a Request (including required fields) or nothing. It's hard to have partly initalized values this way (except users passing null on purpose, but not accidentally, so this does not count to me as a problem).

The disadvantage here is that we require also non-required fields and that can be too much sometimes.

With this solution, in the implementation (like the Transaction class) we can keep the lazy initalization.

skynet2 · 2019-02-24T16:40:01Z

Hi @gregkalapos ,
Not sure that its a good idea. Named tuples are not well supported with json.net JamesNK/Newtonsoft.Json#1230 so we`ll have problems during serialization.

I think it`s easier to expose class with a constructor which requires a list of a parameter (for example url can be one of that parameters) . For example, in order to initialize Request, you should pass Url into a constructor

gregkalapos · 2019-02-24T16:42:10Z

This property would not be serialized, this is just the interface that users use. We serialize the values on the Context. So the ValueTuple property would be marked with [JsonIgnore] on the implementor (Transaction in this case).

gregkalapos · 2019-02-24T16:47:42Z

My problem with having a constructor is that we'll use those classes as properties and it'd mean that can easily end up with null values.

For example:

public interface ITransaction
{
   Request Request { get; set; }
  //...
}

internal class Request : IRequest
{
   public Request(string method, Url url)
   {
      _method = method;
      Url = url;
   }
//...
}

And then in the user code someone writes this:

Agent.Tracer.CurrentTransaction.Request.Method = "GET";

In this case Agent.Tracer.CurrentTransaction.Request can be null.... As a user each time you have to initalize properties. And I think that's not very nice.

skynet2 · 2019-02-24T17:07:38Z

As i understand, in some cases such properties such as Request and Response can be null.
Tuples probably can work... need to think more, probably there are some other ways...

Maybe we should use Builder pattern ?

skynet2 · 2019-02-24T17:12:16Z

As i can see in golang apm they use pretty the same principle https://github.com/elastic/apm-agent-go/blob/master/context.go

gregkalapos · 2019-02-25T10:24:36Z

@skynet2 In the Go code there is only 1 method specifically for the http.Request type, which is a framework type. It's like if we'd add a SetRequest method that accepts Microsoft.AspNetCore.Http.HttpRequest. The big difference is that in Go (at least this is what I was told) http.Request covers almost all the use cases, but in .NET Microsoft.AspNetCore.Http.HttpRequest is used by only 1 framework (ASP.NET Core), and e.g. it does not cover your use-case. So in Go you can't just pass a couple of strings and build your own Request instance.

Regarding the builder pattern: We can discuss that, but I don't see how you force the user on the API level that they pass all required fields. It could be that there is only 1 method and that expects all required properties, but then why do we need the builder pattern? It could be a simple property, and if it's not used we don't send the Request (same as with the builder pattern).

Since this discussion is already drifted from the PR, could we solve this problem here? #124 (it's work in progress, I'll add some potential solutions soon). Once we have a decision there we can continue with the PR here.

gregkalapos

On-hold until we solve the API problem here: #124

Mpdreamz · 2019-02-25T16:38:18Z

To give you an example: we consider renaming Tags to Labels, because we want to confirm to the elastic common schema. By having an interface we can keep Tags and add a new Labels field and mark Tags deprecated but and simply redirect the values to Labels.

@gregkalapos mind explaining this a bit more concretely? This could also be done with a single Request implementation. This also depends on the versioning scheme of the agents vs the server and what the backward compatibility looks like.

If the server renames a property that is a breaking change that should only happen on a major version.

gregkalapos · 2019-02-25T17:39:55Z

To give you an example: we consider renaming Tags to Labels, because we want to confirm to the elastic common schema. By having an interface we can keep Tags and add a new Labels field and mark Tags deprecated but and simply redirect the values to Labels.

@gregkalapos mind explaining this a bit more concretely? This could also be done with a single Request implementation. This also depends on the versioning scheme of the agents vs the server and what the backward compatibility looks like.

If the server renames a property that is a breaking change that should only happen on a major version.

True, this can be also done with a single Request implementation. But in this case the single Request implementation, which is public must conform to the model that the server expects. So we'd have 2 properties, 1 with [JsonIgnore], and then the new one. My goal is to keep the API and the model that the server expects separate.

skynet2 added 3 commits February 18, 2019 22:41

fixes elastic#116

d3a260f

fixes elastic#117

b720849

fixes elastic#119 max span name + handle result

d8b66b2

alvarolobato added the [zube]: In Review label Feb 21, 2019

gregkalapos requested changes Feb 21, 2019

View reviewed changes

src/Elastic.Apm/Api/ISpan.cs Outdated Show resolved Hide resolved

src/Elastic.Apm/Api/ITransaction.cs Outdated Show resolved Hide resolved

src/Elastic.Apm/Model/Payload/Span.cs Outdated Show resolved Hide resolved

gregkalapos mentioned this pull request Feb 21, 2019

Add support for Custom fields #116

Closed

skynet2 and others added 4 commits February 21, 2019 21:56

feat: property max length

73d7f0e

fix: log error

fc6fc90

Add SpanNameLengthTest

45b6890

Merge branch 'master' into feat/fix-span-name

71cbbc8

skynet2 added 7 commits February 23, 2019 15:51

Revert "fixes elastic#116"

875aa4f

This reverts commit 84bcf95.

feat: request\response not null

4192916

feat: basic user

67ec9b0

Merge branch 'feat/fix-span-name' into feature/add-custom

6ce1f1b

feat: trim

2b6866c

feat: user

728a314

feat: body can be an object

e7e6d67

gregkalapos requested changes Feb 24, 2019

View reviewed changes

gregkalapos reviewed Feb 24, 2019

View reviewed changes

src/Elastic.Apm/Elastic.Apm.csproj Outdated Show resolved Hide resolved

skynet2 added 2 commits February 24, 2019 15:37

feat: lazy

14eb21c

feat: styling

e1bde6a

gregkalapos requested changes Feb 24, 2019

View reviewed changes

src/Elastic.Apm/Model/Payload/Context.cs Outdated Show resolved Hide resolved

fix: build

ec85a09

gregkalapos approved these changes Feb 24, 2019

View reviewed changes

fix: string

d5d58a4

gregkalapos self-assigned this Feb 24, 2019

gregkalapos self-requested a review February 25, 2019 10:24

gregkalapos requested changes Feb 25, 2019

View reviewed changes

gregkalapos mentioned this pull request Feb 25, 2019

Expose fields and objects from Transaction.Context through the public agent API #124

Closed

gregkalapos mentioned this pull request Feb 26, 2019

Truncate strings #129

Closed

skynet2 closed this Mar 7, 2019

ycombinator added [zube]: Done and removed [zube]: In Review labels Mar 7, 2019

gregkalapos removed the [zube]: Done label Mar 13, 2019

SergeyKleyman mentioned this pull request Jul 31, 2019

Add support for Custom fields #419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add custom #118

Feature/add custom #118

skynet2 commented Feb 18, 2019 •

edited

Loading

gregkalapos left a comment •

edited

Loading

skynet2 commented Feb 21, 2019

gregkalapos commented Feb 21, 2019

skynet2 commented Feb 23, 2019 •

edited

Loading

gregkalapos left a comment

gregkalapos left a comment

skynet2 commented Feb 24, 2019

gregkalapos left a comment

codecov-io commented Feb 24, 2019 •

edited

Loading

skynet2 commented Feb 24, 2019

gregkalapos commented Feb 24, 2019

skynet2 commented Feb 24, 2019

skynet2 commented Feb 24, 2019

gregkalapos commented Feb 24, 2019 •

edited

Loading

gregkalapos commented Feb 24, 2019 •

edited

Loading

skynet2 commented Feb 24, 2019 •

edited

Loading

gregkalapos commented Feb 24, 2019 •

edited

Loading

gregkalapos commented Feb 24, 2019 •

edited

Loading

skynet2 commented Feb 24, 2019 •

edited

Loading

skynet2 commented Feb 24, 2019

gregkalapos commented Feb 25, 2019

gregkalapos left a comment

Mpdreamz commented Feb 25, 2019

gregkalapos commented Feb 25, 2019

Feature/add custom #118

Feature/add custom #118

Conversation

skynet2 commented Feb 18, 2019 • edited Loading

gregkalapos left a comment • edited Loading

Choose a reason for hiding this comment

skynet2 commented Feb 21, 2019

gregkalapos commented Feb 21, 2019

skynet2 commented Feb 23, 2019 • edited Loading

gregkalapos left a comment

Choose a reason for hiding this comment

gregkalapos left a comment

Choose a reason for hiding this comment

skynet2 commented Feb 24, 2019

gregkalapos left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 24, 2019 • edited Loading

Codecov Report

skynet2 commented Feb 24, 2019

gregkalapos commented Feb 24, 2019

skynet2 commented Feb 24, 2019

skynet2 commented Feb 24, 2019

gregkalapos commented Feb 24, 2019 • edited Loading

gregkalapos commented Feb 24, 2019 • edited Loading

skynet2 commented Feb 24, 2019 • edited Loading

gregkalapos commented Feb 24, 2019 • edited Loading

gregkalapos commented Feb 24, 2019 • edited Loading

skynet2 commented Feb 24, 2019 • edited Loading

skynet2 commented Feb 24, 2019

gregkalapos commented Feb 25, 2019

gregkalapos left a comment

Choose a reason for hiding this comment

Mpdreamz commented Feb 25, 2019

gregkalapos commented Feb 25, 2019

skynet2 commented Feb 18, 2019 •

edited

Loading

gregkalapos left a comment •

edited

Loading

skynet2 commented Feb 23, 2019 •

edited

Loading

codecov-io commented Feb 24, 2019 •

edited

Loading

gregkalapos commented Feb 24, 2019 •

edited

Loading

gregkalapos commented Feb 24, 2019 •

edited

Loading

skynet2 commented Feb 24, 2019 •

edited

Loading

gregkalapos commented Feb 24, 2019 •

edited

Loading

gregkalapos commented Feb 24, 2019 •

edited

Loading

skynet2 commented Feb 24, 2019 •

edited

Loading