Add OperationDetails object bag to DependencyTelemtry for Initializers

[michaelwstark]

Fix Issue # .
[Consolidated]
microsoft/ApplicationInsights-dotnet-server#900

[Original]
microsoft/ApplicationInsights-dotnet-server#820
microsoft/ApplicationInsights-dotnet-server#747
microsoft/ApplicationInsights-dotnet-server#587

• I ran Unit Tests locally.

For significant contributions please make sure you have completed the following items:

• Design discussion issue #
  Support adding arbitrary HTTP response headers from dependencies ApplicationInsights-dotnet-server#587
• Changes in public surface reviewed
• CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
• The PR will trigger build, unit test, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.

[msftclas]

All CLA requirements met.

[michaelwstark]

FWIW. I am not in love with the name of the property -- "Data" was already taken, and putting this on a "Data" field on OperationContext would have made this a much more significant change. Open to suggestions (obviously).

[pharring]

Consider making the Dictionary instantiation lazy to minimize the allocation penalty for code that doesn't need/use it. Also, it should probably be a ConcurrentDictionary (which I'm reluctant to point out because it is an allocation hog)

[lmolkova]

I'm concerned with two things here

1. confusion - we have too many properties already. I imagine users would not understand what info should go here and is it sent to AI backend.
2. abuse - users should not use this dict to pass something unrelated

One possible way to address at least first could be to

1. Make operation details private field
2. Add getter and setter functions
3. mark setter function as not browsable (like TelemetryClient.Track)
4. keep getter public and browsable and give a detailed documentation summary of what it is.

Another way

1. keep OperationDetails public, but not browsable
2. Add extension method in the DependencyCollector package like DependencyTelemtery.TryGetValue(string key)

Even more strict way could be make an enum with possible keys in the DependenycCollector package and allow to get by enum.

[pharring]

For compactness (fewer object allocations), I prefer not to use Lazy<T>.
Instead, make the field non-readonly IDictionary<string, object> and use System.Threading.LazyInitializer.EnsureInitialized.

The pattern is this:

using static System.Threading.LazyInitializer;

private IDictionary<string, object> operationDetails;

public IDictionary<string, object> OperationDetails => EnsureInitialized(ref operationDetails, () => new ConcurrentDictionary<string, object>());

[pharring]

Short-circuit if operationDetails (the backing field) has not been initialized (avoid creating an empty ConcurrentDictionary here).

[cijothomas]

this was meant for 2.6.0 stable or 2.7.0-beta1? @MS-TimothyMothra can help.

[cijothomas]

It'd be great to explicitly mention that these new fields are NOT meant to be serialized and sent to AI backend, they only exist for facilitate TelemetryInitializers.

[michaelwstark]

Let me know if the release the note is under needs to change -- I just put it under the latest one assuming that all pending changes should be put there. If you'd rather not release something in a stable release that hasn't been in a beta release I understand. I'll update the release note and the comment on the set method to explicitly state that the data is not serialized.

[cijothomas]

I think this changes would go into 2.7.0-beta1.
We had already shipped the last beta of 2.6.0 so only critical bugs would be released into 2.6.0-stable. So this change should be in beta1 of the next release, which is 2.7.0-beta1

[pharring]

Lost some indenting here (lines 43-47). Please fix.

[lmolkova]

See comment from the Web SDK here:

microsoft/ApplicationInsights-dotnet-server#897 (comment)

---

The concern is that the feature would postpone GC for the requests/responses since reference would be kept in dependency telemetry until it is sent to the backend (that happens asynchronously).

I wonder, can we clean up the OperationDetails in TrackDependency call after initialize is called to address this concern?

Here

        public void TrackDependency(DependencyTelemetry telemetry)
        {
            if (telemetry == null)
            {
                telemetry = new DependencyTelemetry();
            }

            this.Track(telemetry);

!!!         telemetry.OperationDetails.Clear();
        }

It totally makes sense to me, as Track happens syncronously with dependency call and request/response should not be accessed after that.

[michaelwstark]

Seems reasonable. I'll have to add some additional work to the test verification in the other repo, but shouldn't be to hard to grab these items prior to them being cleared out.

[michaelwstark]

I expected my tests for the dependency collector package to start failing, and when they did not, I investigated and found TrackDependency() is never called -- Track() is called directly. Is there a reason TrackDependency is not being called?

[lmolkova]

I expected my tests for the dependency collector package to start failing, and when they did not, I investigated and found TrackDependency() is never called -- Track() is called directly. Is there a reason TrackDependency is not being called?

You are right, we don't actually call TrackDependency, and I don't think there is any reason behind it as it is equivalent to Track.

Ok so the alternative could be to clean OperationDetail in the Web SDK (in dependency collector) where we assign them after the track call...

[michaelwstark]

Ok so the alternative could be to clean OperationDetail in the Web SDK (in dependency collector) where we assign them after the track call...

@lmolkova - I actually prefer the original suggestion (seems like less maintenance over time), and had already updated the web sdk repo to call TrackDependency(), I was just verifying that there was not a reason for this. I noticed that Track() itself is not browsable.

[lmolkova]

I actually prefer the original suggestion (seems like less maintenance over time), and had already updated the web sdk repo to call TrackDependency(), I was just verifying that there was not a reason for this. I noticed that Track() itself is not browsable.

Sure, calling TrackDependency instead of Track is fine

[cijothomas]

Manually triggered CI build/tests - all good and ready to merge

[cijothomas]

@MS-TimothyMothra - Please merge this when branch is open for 2.7.0-beta1

[MikeEvansLarah]

Is there any way to retrieve the request body for HTTP dependency calls using the OperationDetails?

Following the links posted earlier, I find that I can retrieve the HttpWebRequest in a telemetry initializer like so:

if (!(telemetry is DependencyTelemetry dependency))
{
    return;
}

if (dependency.TryGetOperationDetail("HttpRequest", out var request)
    && request is HttpWebRequest httpWebRequest)
{
    if (httpWebRequest.Method == "POST" || httpWebRequest.Method == "PUT")
    {
        // get request body somehow?
        dependency.Properties.Add("RequestBody", body);
    }
}

But the stream on the request is closed at this point, so unable to be read.

Alternatively, is there any other way to capture the request body such that it is attached to the dependency telemetry?

[lmolkova]

@MikeLarah no, there is no way to read closed stream.
You may log the request body at the place where it is created and written to the request stream, you may log it with TelemetryClient.TrackTrace(). It is problematic to make it a child of the dependency, by default, it will be the child of the request