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

[gregkalapos]

Why?

The current public agent API only exposes a small part of the model that the APM Server expects. These are mainly things directly on Transaction and Span. But Transaction.Context and Span.Context are not exposed. On those contexts we can set things like HTTP request related fields (HTTP method, URL, etc.) or Database related fields.

Since there are lots of libraries that we currently don't support with auto instrumentation we should expose these fields and let users set those when they rely on the public agent API.

This PR already contains a use case: #118

Problem description

To avoid a too high-level discussion, I'll focus specifically on Transaction.Context.Request, but we should came up with a solution that works with other things on Transaction.Context and Span.Context.

Request has 2 required fields:

• Method
• Url

This means that both of these fields must be set, otherwise the the server would reject the Transaction.

Requirements

(not set it stone, feel free to disagree)

• The main challenge is that we have to make sure that required fields are filled, otherwise the APM server would reject the request. This means we should not have partially initialized objects. E.g.: when the user does this: Agent.Tracer.CurrentTransaction.Request.Method = "GET"; We could lazily create the Request object, but the problem is that the Url property is still null, so if the user does not set that property later then this Request object is invalid.

• Avoid throwing exceptions. For example with the previous example (where the server'd reject the transaction, since not all required fields are filled on Transaction.Context.Request) we could build some verification before we send the data to the apm server (or at another point) and notify the user by throwing an exception and forcing them to avoid partly initialized data. As a monitoring tool one basic principle is to be as non-intrusive as possible, so I think throwing exceptions is not acceptable.

• Avoid skipping data. Another option would be to not send that data and print a warning (or any other log message). I think this'd cause confusion.

• Required fields should be forced on the API level. We should prefer solutions where it's not even possible to have partly initilized objects. This'd mean the user either sets all the required fields at once, or none of those.

• There should be no NullReferenceExceptions in case something is not initialized. For example if we have let's say an API where the Request is a property, then something like this should not throw an exception (even if the user did not initialize the Request property before): Agent.Tracer.CurrentTransaction.Request.Method = "GET"; Similarly this should also never throw an exception: var requestMethod = Agent.Tracer.CurrentTransaction.Request.Method

Potential solutions:

1. Lazy initialization (original attempt)

public interface ITransaction
{
   IRequest Request { get; }
}

internal class Transaction : ITransaction
{
  public IRequest Request => _context.Value?.Request;
}

internal class Context
{
  private readonly Lazy<Request> _request = new Lazy<Request>();
  public Response Response => _response.Value;
}

Advantage:

• No NullReferenceException, if the user writes this: Agent.Tracer.CurrentTransaction.Request.Method we immediately initalize a Request

Disadvantage:

• This can cause partially initialized data. (the line above already does that).

2. Exposing 2 methods on ITransaction:

• Setter: Required parameters for required fields and has multiple optional parameters for optional fields
• Getter: Uses ValueTuple and nullchecks if the user haven't initialized the Request it returns default values if it's non-initialized, otherwise it returns the values.

public interface ITransaction
{
   void SetRequest(string method, string urlProtocol, string urlFull, string urlHostName, 
      string urlRaw = null, bool socketEncrypted = false, string socketRemoteAddress = null,
      string httpVersion = null, object body = null);

   (string method, string urlProtocol, string urlFull, string urlHostName, string urlRaw, 
      bool socketEncrypted, string httpVersion, object body) GetRequest();
}

internal class Request
{
 // with a ctor we force to pass required fields. 
  public Request(Url url, string method)
  {
    Url = url;
    Method = method;
  }	
  public string HttpVersion {//get&set}
  public object Body {//get&set}
  public string Method {//get&set}
  public Socket Socket { get; set; }
  public Url Url { get; set; }
}


internal class Transaction : ITransaction
{
  public void SetRequest(string method, string urlProtocol, string urlFull, string urlHostName, 
      string urlRaw = null, bool socketEncrypted = false, string socketRemoteAddress = null, 
      string httpVersion = null, object body = null)
     => _context.Value.Request = new Request(new Url() { Full =  urlFull, Protocol =  urlProtocol, 
         Raw =  urlRaw, HostName =  urlHostName}, method){Body =  body, Socket
          =  new Socket() { Encrypted =   socketEncrypted, RemoteAddress =  socketRemoteAddress},
         HttpVersion =  httpVersion};
   
}

public (string method, string urlProtocol, string urlFull, string urlHostName, string urlRaw, bool socketEncrypted,
  string httpVersion, object body)
    GetRequest() => _context.Value.Request == null ?
      (string.Empty, string.Empty, string.Empty, string.Empty, string.Empty, false, string.Empty, null) :
        (_context.Value.Request.Method, _context.Value.Request.Url.Protocol, _context.Value.Request.Url.Full,
          _context.Value.Request.Url.HostName, _context.Value.Request.Url.Raw, _context.Value.Request.Socket.Encrypted,
          _context.Value.Request.HttpVersion, _context.Value.Request.Body);

Here is how the user code'd look like:

transaction.SetRequest(method: "GET", urlProtocol: "HTTP", urlFull:"https://myUrl.com", urlHostName: "MyUrl", httpVersion: "2.0");
				
 //later in the code:
var request = transaction.GetRequest();
Console.WriteLine(request.method);
Console.WriteLine(request.httpVersion); //etc...

Advantages:

• The API forces the user to only create fully initialized objects
• There are no NullReferenceExceptions, if someone writes this var request = transaction.GetRequest().method we just return string.empty. In case of object we return null, but these are always fields that are at the end of the chain...
• We don't need interfaces... there is no IRequest or IUrl, since the Request and Url objects are completely hidden.

Disadvantage:

• Since there is 1 setter and 1 getter users have to set everything in a single step. If e.g. the body is available only at a later point then the whole call must be moved to a later point in the code.
• We can't extend the getter... there is nothing like overloads for ValueTuples as return types... the number of parameters for GetRequest() is fixed.

One modification of this approach would be to use specific types as parameters.

3. Introducing intermediate types that work as public API

Implemented in #130

4. exposing the intake API as it is.

Implemented in #134

@elastic/dotnet: maybe someone has an idea, opinion or just a bright comment.

[Mpdreamz]

Yes NullReferenceException is bad but we should not be too clever here IMO.

If you program directly with the public API nulls (just like anything else with C#) should be part of the game. I particularly dislike returning empty strings for properties that were not set.

public class Transaction
{
    internal Context Context { get; } = new Context();
    public Request Request 
    {
        get=> Context.Request;
        set => Context.Request = value;
    }
}

public class Request 
{
  public Request(Url url, string method) => (Url, Method) = (url, method);

  public string Method { get; }
  public Url Url { get; }

  public string HttpVersion { get; set; }
  public object Body { get; set; }
  public Socket Socket { get; set; }
}

The statement that the following should never throw:

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

Almost forces us to a design pattern that would be a poor implementation of functional lenses in C#. I don't think making this a requirement does our users any good in the long run. The nodejs agent also documents the fact some properties can be null if we need prior art to validate this design choice.

[gregkalapos]

The statement that the following should never throw:

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

Almost forces us to a design pattern that would be a poor implementation of functional lenses in C#.

I agree on that. If we have a property chain like that then it's impossible to avoid nullrefs, or it'd be very strange. My intention with saying this was rather this: maybe we should find a way to avoid such properties. And the 2. proposed solution avoids that property chain.

I think we can consider your code snippet as a 3. solution.

Here is what I think:
Advantage:

• Very clean, no special magic
• It avoids partly initialized objects

Disadvantage:

• The NullRef problem. E.g. the example you bring from the node agent is a scenario where an agent method returns something which is null. I think that's acceptable, and that's fine, but in this case we would throw an exception within the agent API with Agent.Tracer.CurrentTransaction.Request.Method, to me this is a little bit different.

A variation of this would be to not expose model classes that are expected by the server, but using API classes instead. I'd consider this option.

Also to:

I particularly dislike returning empty strings for properties that were not set.

We can also return nulls instead of empty string... the point of the 2. proposed solution is to hide the model type and force the users to pass every required field on the API level, but not to avoid null in the "end of the property chain". I'm totally fine with returning (null, null, null, ... , null).

[gregkalapos]

Discussed this with @SergeyKleyman and @Mpdreamz.

Decision: We expose the model that we have to the APM server with the Intake API as it is and we won't introduce any intermediate layer on the API. (proposed solution 4.)

Mainly because adding an intermediate layer (like in #130) would mean that we have to maintain a mapping, which is not trivial.

Regarding breaking changes: we assume that on the intake API we will only have breaking changes on major versions and in those cases we will also have breaking changes in the C# API which is acceptable.

The other advantage is that everything which is in the server docs and in the intake API doc automatically applies to the .NET API.

cc: @elastic/apm-agent-devs @roncohen We decided to expose most part of the intake API as it is in the .NET API (e.g.: transaction.context.request can be directly set in C#). If someone has a good argument to not to do this, this would be a good time to let us know.

[watson]

For what it's worth, the Node.js agent allows users to modify any part of the payload before it's sent to the APM Server - not only context.request - through filters: https://www.elastic.co/guide/en/apm/agent/nodejs/current/agent-api.html#apm-add-filter

[gregkalapos]

For what it's worth, the Node.js agent allows users to modify any part of the payload before it's sent to the APM Server - not only context.request - through filters: https://www.elastic.co/guide/en/apm/agent/nodejs/current/agent-api.html#apm-add-filter

Nice! Thx @watson!

In order to implement this in .NET we need to make those types (context, request, etc.) public anyway, so this is aligned with our plan. Whether we build a similar concept is a different question, but it's worth to consider. But as the prerequisite first we need to expose our types.

[skynet2]

Probably the best solution!
Thanks, as for me, a developer should be responsible for what he is doing, Apm client should have good error logging, so we can see errors from apm server.
Is this possible to use somehow https://docs.microsoft.com/en-ca/dotnet/api/microsoft.extensions.logging.ilogger-1?view=aspnetcore-2.2 ?
current implementation for logger is only for console

[gregkalapos]

Probably the best solution!
Thanks, as for me, a developer should be responsible for what he is doing, Apm client should have good error logging, so we can see errors from apm server.
Is this possible to use somehow https://docs.microsoft.com/en-ca/dotnet/api/microsoft.extensions.logging.ilogger-1?view=aspnetcore-2.2 ?
current implementation for logger is only for console

Thanks for the feedback @skynet2!

Regarding logging: #61 is about exactly that topic. We plan to offer more than a console logger, we already have most of the infrastructure for that.

[gregkalapos]

PR ready for review in #134.

[axw]

@gregkalapos I don't know how much of this applies to .NET, but since you asked: I originally had exposed the model types in the Go agent, and ended up hiding them and adding an API.

The main reason for was performance. The model types in the Go agent store some of the data in unconventional ways in order to minimise memory allocations in the first place, enable object pooling/reuse, and to speed up JSON encoding. We hide all of this behind a more conventional API, doing the conversions and object reuse behind the scenes.

It also means that the breaking model changes don't necessarily require breaking the API and user's code. While that should be fairly infrequent, it's still not nice and may be an impediment to users (especially third-party instrumentation modules) upgrading.

[felixbarny]

We hide all of this behind a more conventional API, doing the conversions and object reuse behind the scenes.

Does the API allow to set all fields of the intake API? Is it possible to retrieve values from it?

It also means that the breaking model changes don't necessarily require breaking the API and user's code.

That's a good point. Especially considering the uncertainty of API changes related to conforming to ECS like whether or not we want to drop context, rename tags to labels etc.

[axw]

Does the API allow to set all fields of the intake API? Is it possible to retrieve values from it?

There are a couple of fields which I didn't provide an API for, because they aren't sensible for Go. Otherwise you can set them all. Sometimes it's a little bit indirect, like you would use transaction.Context.SetHTTPRequest(request) to set the request URL, method, headers, version, cookies, and socket.

In general, the API is write-only. There's a handful of exceptions (transaction name, type, duration, and result), which are fields primarily for aesthetics. They aren't directly part of the model type, but get copied across at serialisation time. It's worth noting that the Go agent does not have any kind of "filter" mechanism whereby users can transform the events.

[felixbarny]

The main reason for was performance. The model types in the Go agent store some of the data in unconventional ways in order to minimise memory allocations in the first place, enable object pooling/reuse, and to speed up JSON encoding.

This is a consideration for Java as well. We're using types which facilitate object reuse like CharBuffers, StringBuilders etc. But we don't want to expose those as it would introduce too tight coupling and increases the risk of breaking changes.

Currently, we don't have an option to set context fields via the API at all but we are considering options to allow for that. It would also be a write-only API so implementing filters would be challenging.

[gregkalapos]

The main reason for was performance. The model types in the Go agent store some of the data in unconventional ways in order to minimise memory allocations in the first place, enable object pooling/reuse, and to speed up JSON encoding.

I see! Interesting. In .NET I don't see any reason for having an extra layer in order to do this. We can do pooling/reuse also when we expose the types that map 1-1 to the server API. I guess this is more like a Go specific thing.

It also means that the breaking model changes don't necessarily require breaking the API and user's code.

That's a good point. Especially considering the uncertainty of API changes related to conforming to ECS like whether or not we want to drop context, rename tags to labels etc.

I personally agree on this one very much, that was the main intention to my original PR (#130). But when we discussed this with @Mpdreamz and @SergeyKleyman we agreed, that those breaking changes only should happen on minor major server version changes, and we are ok with breaking the API in that case. Another advantage to expose the server API is that if someone looks at the json schema they can immediately use the Agent API, because it's the same. But I personally agree on this one.

@felixbarny what's the plan in Java? I think most of the stuff isn't exposed currently, right? Update: Sorry, have internet trouble, I see your answer

Let me sum up what we learned until now:

• Node.js: uses filters and basically exposes the server API through those (similar to the direction in Expose Transaction.Context.Request and Transaction.Context.Response #134)
• Go: has an intermediate layer (It'd be something like Expose Transaction.Context.Request through the public agent API #130) - write only
• Java: not exposing Context atm.

[Mpdreamz]

we agreed, that those breaking changes only should happen on minor server version changes...

Small correction major version changes on the server. :)

[gregkalapos]

we agreed, that those breaking changes only should happen on minor server version changes...

Small correction major version changes on the server. :)

👍 Yes, typo, sorry.

[felixbarny]

In .NET I don't see any reason for having an extra layer in order to do this. We can do pooling/reuse also when we expose the types that map 1-1 to the server API. I guess this is more like a Go specific thing.

One good example where the internal representation of the Java agent deviates from the JSON is Transaction.context.request.body. We differ between three cases:

• The body is a string, like [REDACTED]
• The body is url-formencoded (form parameters). For this case we use a special Map-like data structure which lets us add entries without allocating a Map.Entry object. If there is only one value for a key, it serializes to "key": "value", whereas multiple values serialize to "key": ["value1", "value2"] to conform with how the Node.js agent handles parameters. It also avoids allocating a list incase there is only a single value.
• In case it's a non-url-formencoded request body, we use CharBuffer to record the body. There's no way in the Java Servlet API to directly get the body as a string. We have to read the bytes of the body stream and encode them on the fly into the CharBuffer. In order to avoid GC pressure, we reuse the CharBuffer objects (separately from reusing the Transaction object).

As you can see, there is no 1:1 mapping of the internal model and the intake API. The used types are also quite unconventional and might change at minor or even bugfix versions. This is highly Java specific but maybe you have similar challenges in .NET.

[gregkalapos]

Yes, we also have this challenge in .NET.

But is this really related to public vs. private API?

If I understand correctly you just store body as object and you return either the CharBuffer or the other thing depending on what's stored internally. https://github.com/elastic/apm-agent-java/blob/master/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/Request.java#L92

I guess in .NET we would just expose it as it is. We can discuss whether exposing object is a good idea (maybe not :) ). This'd be something like a discriminated union - but we don't have it in C#.

Do you suggest to solve this with a class that is part of the public API and solves the issue with a different type that is smarter and hides the real request.body somehow?

[felixbarny]

The Java agent does not expose the different body data structures at all. What you have linked to is the internal API. The public API could abstract this by just allowing to set the body as a String, for example. This is similar to the Span name. Internally, it's represented as a StringBuilder this allows to append different parts to the name, like name.append(method).append(" ").append(host) without allocating objects by concatenating strings. The public API however only has a setName(String) method, not offering access to the internal StringBuilder. That makes it easier to make internal changes, like using a CharBuffer or just a String instead of a StringBuilder, without introducing breaking changes.

[gregkalapos]

Got it! Yeah, that's how I imagined.

For .NET people:
We could still do this with a single class.

public class Span
{
     [JsonIgnore] //internal is ignored anyway...
     internal StringBuilder NameInternal {get; set; } = new StringBuilder();
   
     public string Name { get => _sb.ToString(); set => _sb = new StringBuilder(value); }
}

Everything that is public would 1-1 map to the server API, the reming stuff is only used internally and nothing is serialized from that.
With this:

• public API: basically the server json schema as public properties on public classes.
• internal API: public API + all the optimization stuff we need as internal properties and methods.

Note: I personally still lean towards separating the server API model classes from the public API by having different classes in different namespaces, so I just discuss here according to our common decision where we outvoted #130.

[gregkalapos]

Merged #134. That covers Transaction.Context.
Will continue with the same strategy on Span.

[gregkalapos]

Done and merged.

We went with "4. exposing the intake API as it is.". Both Transaction.Context and Span.Context are exposed to users through the Public Agent API.