Track dependency instrumented with HttpDesktop DiagnosticSource in Response event (#509) #516

lmolkova · 2017-04-28T01:05:40Z

See #509 for the details.

Please note:

Before this fix, all dependencies are reported, but none of the dependencies instrumented with DiagnosticSource have proper Target with appId.
After this fix

dependency has proper target if AppId was returned in response header
for requests with redirects, only first call is tracked (so the duration and status are wrong). This we should fix in DiagnosticSource. (Update fixed and merged)
some dependency calls may be missed if they are done with WebRequest.GetResponse() AND response is not disposed. This is fundamental flaw in the reflection hook and there is no know solution for this issue.
E.g.

            HttpWebRequest request = WebRequest.CreateHttp("http://localhost:3000");
            HttpWebResponse response = (HttpWebResponse)request.GetResponse();

Will never be tracked... Application that does not dispose WebRequests may run short on the available connections, so in most cases uses will dispose them.

            HttpWebRequest request = WebRequest.CreateHttp("http://localhost:3000");
            using( HttpWebResponse response = (HttpWebResponse)request.GetResponse())
            {
            }

Will be tracked.
This problem does not affect requests done with HttpClient as responses are disposed.

Considering that some dependencies may not be collected and it does not look fixable, please decide whether this fix should be taken at all.
Otherwise, just keep current implementation (and sacrifice AppId for all dependencies).

dnduffy · 2017-04-28T02:03:27Z

Src/DependencyCollector/Shared.Tests/Implementation/FrameworkHttpProcessingTest.cs

            DependencyTelemetry remoteDependencyTelemetryActual, Uri url, string kind, bool? success, double valueMin, string statusCode)
        {
            Assert.AreEqual("GET " + url.AbsolutePath, remoteDependencyTelemetryActual.Name, true, "Resource name in the sent telemetry is wrong");
            string expectedVersion = SdkVersionHelper.GetExpectedSdkVersion(typeof(DependencyTrackingTelemetryModule), prefix: "rddfd:");
            ValidateTelemetryPacket(remoteDependencyTelemetryActual, url, kind, success, valueMin, statusCode, expectedVersion);
-        }
+        }*/


Just delete unneeded code.

nice catch!
forgot to remove it

dnduffy · 2017-04-28T02:04:59Z

Src/DependencyCollector/Shared/Implementation/DesktopDiagnosticSourceHttpProcessing.cs

+        internal ObjectInstanceBasedOperationHolder TelemetryTable;
+
+        internal DesktopDiagnosticSourceHttpProcessing(TelemetryConfiguration configuration, ObjectInstanceBasedOperationHolder telemetryTupleHolder, bool setCorrelationHeaders, ICollection<string> correlationDomainExclusionList, string appIdEndpoint)
+            : base(configuration, SdkVersionUtils.GetSdkVersion("rdd" + RddSource.FrameworkAndDiagnostic + ":"), null, setCorrelationHeaders, correlationDomainExclusionList, appIdEndpoint)


Please check with Sergey if "rdd" should be some other prefix code for uniqueness or not.

it was already there (RddSource.FrameworkAndDiagnostic) for this source

From this PR, it looks like DiagnosticSource and Framework cannot be active at the same time. So RddSource.FrameworkAndDiagnostic does not make sense here as it was originally intended to tell us dependency was collected by both Framework and Diagnostic source.

dnduffy

Minor changes/questions.

lmolkova · 2017-04-28T02:33:20Z

Dan Taylor suggestion is to have a flag that would let users choose what they want to sacrifice :)

cijothomas · 2017-04-28T19:40:46Z

Src/DependencyCollector/Net46.Tests/DependencyTrackingTelemetryModuleTestNet46.cs

+                using (var reader = new StreamReader(response.GetResponseStream()))
+                {
+                    reader.ReadToEnd();
+                }


Please retain the test for 'GetResponse() but no subsequent stream read' to test that we wont collect dependencies in that case, and having a test with comments will be easier for future reference.

cijothomas · 2017-04-28T19:46:21Z

...ependencyCollector/Shared.Tests/Implementation/DesktopDiagnosticSourceHttpProcessingTests.cs

+        [TestMethod]
+        public void RddTestHttpProcessingFrameworkUpdateTelemetryName()
+        {
+            var stopwatch = Stopwatch.StartNew();


start stopwatch right before this line 'this.httpProcessingFramework.OnRequestSend(request); Don't want to include the time taken (its too small but still!) for WebRequest.Create.

same for all other tests in this class.

cijothomas · 2017-04-28T20:36:54Z

@dnduffy How important is to get this 2.4.0-beta2 for you to refer in aspnet core? This change is non-trivial, and dont want to include this in beta2 without routine manual tests. Can this wait for 2.4 stable release ?

@lmolkova - Will need to review this in detail+ check functional tests before merging this in.

lmolkova · 2017-04-28T20:38:25Z

@cijothomas This is for Web SDK, it is quite important for the //build scenarios and stable 2.4.0.

lmolkova · 2017-04-28T23:19:33Z

Update on the issue with not-tracked calls: they will be tracked if the response is closed/disposed properly.

            HttpWebRequest request = WebRequest.CreateHttp("http://localhost:3000");
            using (HttpWebResponse response = (HttpWebResponse)request.GetResponse())
            {
                Console.WriteLine(response.StatusCode);
            }

If app does not dispose WebRequests properly, it could experience a lot of other issues: e.g. reach connection limit.

So I do not consider this is a critical problem anymore even for WebRequests

lmolkova · 2017-05-01T21:53:21Z

Hey guys, please merge it to develop if possible.
@cijothomas @SergeyKanzhelev

cijothomas · 2017-05-02T06:35:29Z

...ependencyCollector/Shared.Tests/Implementation/DesktopDiagnosticSourceHttpProcessingTests.cs

+            this.sendItems = new List<ITelemetry>();
+            this.configuration.TelemetryChannel = new StubTelemetryChannel { OnSend = item => this.sendItems.Add(item) };
+            this.configuration.InstrumentationKey = Guid.NewGuid().ToString();
+            this.httpProcessingFramework = new DesktopDiagnosticSourceHttpProcessing(this.configuration, new ObjectInstanceBasedOperationHolder(), /*setCorrelationHeaders*/ true, new List<string>(), RandomAppIdEndpoint);


httpProcessingFramework - this name is misleading. Please change to reflect the new class name.

This is not changed. DesktopDiagnosticSourceHttpProcessingTests is not testing HttpProcessingFramework, but the variable name is telling otherwise. (its difficult to read code with consuing names :( )

cijothomas

Some comments left. Most importantly - can you add/modify Functional tests for the changes? There are a bunch of Functional tests already for dependency collector , please make use of that to validate things end to end.

cijothomas · 2017-05-02T06:47:44Z

need to discuss about the version prefix. RddSource.FrameworkAndDiagnostic does not seem logical here, rddd maybe okay, but DependencyCollector in Core already uses that, so we need to distinguish that as well.

nizarq · 2017-05-02T09:27:23Z

Src/DependencyCollector/Shared/DependencyTrackingTelemetryModule.cs

@@ -221,9 +221,23 @@ protected virtual void Dispose(bool disposing)
        /// </summary>
        private void InitializeForFrameworkEventSource()
        {
+#if NET45


What is the difference between #if NET 45 here and #if !NET40 below. Are we dealing with .net framework < 40?

you are right, there is no difference

lmolkova · 2017-05-02T17:26:48Z

need to discuss about the version prefix. RddSource.FrameworkAndDiagnostic does not seem logical here, rddd maybe okay, but DependencyCollector in Core already uses that, so we need to distinguish that as well.

Core and Desktop will never be active at the same time, but it could be useful to distinguish them anyway. What about 'rdddd' (for rdd desktop diagnostics)? However it seems too similar to 'rddd' used in core.
Please advise what prefix is appropriate. I'm fine with anything.

lmolkova · 2017-05-02T17:36:51Z

Some comments left. Most importantly - can you add/modify Functional tests for the changes? There are a bunch of Functional tests already for dependency collector , please make use of that to validate things end to end.

There are several difficult aspects of adding/changing functional tests

There is no NET46 app and this instrumentation only works for NET46.
In order to test it end-to-end, we need 2 apps: 1st app that tracks dependency, second app that responds with Request-Context headers.

I would prefer another solution described in #494, so we can have local mocked server and track dependencies done to it. It requires a bit of investment to set up localhost. There are other questions where those tests should live: in unit-tests projects or in functional tests. I would argue to keep them as unit tests.
Anyway, I agree that there should be e2e tests, I agree to add them, however I would take it separately as additional PR, which I can do not earlier than a week from now. For now, end-to-end scenarios for these changes were tested manually.

nizarq · 2017-05-02T19:18:15Z

Src/DependencyCollector/Shared/Implementation/DesktopDiagnosticSourceHttpProcessing.cs

+    /// </summary>
+    internal sealed class DesktopDiagnosticSourceHttpProcessing : HttpProcessing
+    {
+        internal ObjectInstanceBasedOperationHolder TelemetryTable;


Why make this internal? For tests? Should we make this a private field and expose an internal property instead? It's best practice to have fields private only - properties give more fine grained control on exposing what you want to expose to the outside world - for example may be you only want to give get permissions not set.

copy/paste issue

cijothomas · 2017-05-02T19:25:01Z

...ependencyCollector/Shared.Tests/Implementation/DesktopDiagnosticSourceHttpProcessingTests.cs

+        /// Validates that OnRequestSend and OnResponseReceive sends valid telemetry.
+        /// </summary>
+        [TestMethod]
+        public void RddTestHttpProcessingFrameworkUpdateTelemetryName()


can you modify all the test names to reflect what they are testing? HttpProcessing vs DesktopDiagnosticsSourceHttpProcessing?

lmolkova · 2017-05-02T20:34:29Z

Update: redirect issue is fixed in DiagnosticSource

cijothomas · 2017-05-03T15:54:12Z

Src/DependencyCollector/Shared/DependencyTrackingTelemetryModule.cs

@@ -221,9 +221,21 @@ protected virtual void Dispose(bool disposing)
        /// </summary>
        private void InitializeForFrameworkEventSource()


This name does not hold true anymore as we are not only initializing frameworkeventsource but initializing DiagnosticSource as well.

cijothomas · 2017-05-03T15:56:05Z

Src/DependencyCollector/Shared/Implementation/DesktopDiagnosticSourceHttpProcessing.cs

+    /// <summary>
+    /// Concrete class with all processing logic to generate RDD data from the callbacks received from HttpDesktopDiagnosticSourceListener.
+    /// </summary>
+    internal sealed class DesktopDiagnosticSourceHttpProcessing : HttpProcessing


Won't this receive callbacks for failure cases? I only see logic to handle successfull dependencies.. (FrameworkEventSource, profiler had logic for failed calls)

Could you please show me where FrameworkEventSource reports failure cases?
I don't see it here here

And in case of HTTP failure means exception because of e.g. DNS or network issue or cancellation.

cijothomas · 2017-05-03T16:05:02Z

Src/DependencyCollector/Shared/Implementation/HttpDesktopDiagnosticSourceListener.cs

@@ -53,7 +53,7 @@ public void OnNext(KeyValuePair<string, object> value)
                    case "System.Net.Http.Request": 
                    {
                        var request = (HttpWebRequest)this.requestFetcherRequestEvent.Fetch(value.Value);


We need to add DependencyCollectorEventSource.Log.Callback received type of logging here.

cijothomas · 2017-05-03T16:09:00Z

Src/DependencyCollector/Shared/Implementation/HttpProcessing.cs

@@ -249,8 +249,7 @@ internal object OnBegin(object thisObj, bool skipIfNotNew)
        /// <param name="exception">The exception object if any.</param>
        /// <param name="thisObj">This object.</param>                
        /// <param name="returnValue">Return value of the function if any.</param>
-        /// <param name="endTracking">Whether this method should end the tracking or not.</param>
-        internal void OnEnd(object exception, object thisObj, object returnValue, bool endTracking)
+        internal void OnEnd(object exception, object thisObj, object returnValue)


HttpProcessing class should not log about callbacks. Do that in the corresponding listeners.

Callbacks were not introduced in this PR, moving them goes far beyond this pull request,. This PR a critical issue fix and I doubt we should do more refactoring here

I added callback logs into the listener and created an issue to consistently change profiler/httpprocessing logs #537

cijothomas · 2017-05-03T16:11:02Z

Src/DependencyCollector/Shared/DependencyTrackingTelemetryModule.cs

-            FrameworkHttpProcessing frameworkHttpProcessing = new FrameworkHttpProcessing(this.telemetryConfiguration, DependencyTableStore.Instance.WebRequestCacheHolder, this.SetComponentCorrelationHttpHeaders, this.ExcludeComponentCorrelationHttpHeadersOnDomains, this.EffectiveProfileQueryEndpoint);
-            this.httpDesktopDiagnosticSourceListener = new HttpDesktopDiagnosticSourceListener(frameworkHttpProcessing);
+#if NET45
+            DesktopDiagnosticSourceHttpProcessing desktopHttpProcessing = new DesktopDiagnosticSourceHttpProcessing(


we are now enabling both diagnostic source and framework event source. where is the logic to prevent double collection or only one will be active at a time?

it's here

In runtime, we do not know what happens first: initialization of Http Desktop DiagnosticSource or initialization of the DependencyTrackingTelemetryModule, so we keep listening to both, but ignoring the EventSource if DiagnosticSource is active.

DiagnosticSource Response event (#509)

…ault

cijothomas

Please check all the comments. There are lot.

cijothomas · 2017-05-04T16:32:15Z

Src/DependencyCollector/Shared/DependencyTrackingTelemetryModule.cs

@@ -47,6 +47,11 @@ public class DependencyTrackingTelemetryModule : ITelemetryModule, IDisposable
        public bool DisableRuntimeInstrumentation { get; set; }

        /// <summary>
+        /// Gets or sets a value indicating whether to disable runtime instrumentation.


minor: incorrect comment.

cijothomas · 2017-05-04T16:38:05Z

Src/DependencyCollector/Shared/DependencyTrackingTelemetryModule.cs

+                    this.ExcludeComponentCorrelationHttpHeadersOnDomains,
+                    this.EffectiveProfileQueryEndpoint);
+                this.httpDesktopDiagnosticSourceListener = new HttpDesktopDiagnosticSourceListener(desktopHttpProcessing);
+            }


please add a log here saying DesktopDiagnosticSource was not enabled as it was not enabled in config. Helps in debugging customer issues later.

Let's add log that is WAS actually activated.

cijothomas · 2017-05-04T16:41:19Z

Src/DependencyCollector/Shared/Implementation/HttpDesktopDiagnosticSourceListener.cs

    using System.Net;

    /// <summary>
    /// Diagnostic listener implementation that listens for Http DiagnosticSource to see all outgoing HTTP dependency requests.
    /// </summary>
    internal class HttpDesktopDiagnosticSourceListener : IObserver<KeyValuePair<string, object>>, IDisposable
    {
-        private readonly FrameworkHttpProcessing httpProcessingFramework;
+        private readonly DesktopDiagnosticSourceHttpProcessing httpDesktopProcessingFramework;


minor - rename variable to reflect the class name. (Framework at the end will be confusing_)

cijothomas · 2017-05-04T16:56:19Z

Src/DependencyCollector/Shared/Implementation/FrameworkHttpProcessing.cs

+                if (DependencyTableStore.Instance.IsDesktopHttpDiagnosticSourceActivated)
+                {
+                    // request is handled by Desktop DiagnosticSource Listener
+                    DependencyCollectorEventSource.Log.TrackingAnExistingTelemetryItemVerbose();


please use a different log message here. TrackingAnExistingTelemetryItemVerbose() is used for a different purpose (it existed before diagnostic source). Log something like "I am not tracking this, as DesktopDiagnosticSource is already enabled"

I've already done that in the next PR #538, there was a special event added there

cijothomas · 2017-05-04T17:14:20Z

Src/DependencyCollector/Shared/Implementation/HttpDesktopDiagnosticSourceSubscriber.cs

@@ -48,6 +48,7 @@ public void OnNext(DiagnosticListener value)
                if (value.Name == "System.Net.Http.Desktop")
                {
                    this.sourceSubscription = value.Subscribe(this.parent, (Predicate<string>)null);
+                    DependencyTableStore.Instance.IsDesktopHttpDiagnosticSourceActivated = true;


Does it need to set this back to false on Dispose?

cijothomas · 2017-05-04T17:38:20Z

Src/DependencyCollector/Shared/DependencyTrackingTelemetryModule.cs

                        DependencyCollectorEventSource.Log.ProfilerFailedToAttachError(exp.ToInvariantString());
                    }
                }
                else
                {
                    // if config is set to disable runtime instrumentation then default to framework event source
-                    this.InitializeForFrameworkEventSource();
+                    this.InitializeForDiagnosticAndFrameworkEventSource();
                    DependencyCollectorEventSource.Log.RemoteDependencyModuleVerbose("Runtime instrumentation is set to disabled. Initialize with framework event source instead.");
                }
            }
            else
            {
                // if profiler is not attached then default to framework event source


minor - change comment to indicate eventsouce and/or diagnostic listene.

cijothomas · 2017-05-04T17:49:33Z

Src/DependencyCollector/Shared/Implementation/ClientServerDependencyTracker.cs

@@ -165,7 +165,7 @@ internal static void AddTupleForWebDependencies(WebRequest webRequest, Dependenc

            Tuple<DependencyTelemetry, bool> telemetryTuple = null;

-            if (DependencyTableStore.Instance.IsProfilerActivated || PretendProfilerIsAttached)
+            if (DependencyTableStore.Instance.IsDesktopHttpDiagnosticSourceActivated || DependencyTableStore.Instance.IsProfilerActivated || PretendProfilerIsAttached)
            {


This does not look right. This means that, even when Profiler is not installed, tuple will be looked up in SQLRequestConditionalHolder - this is incorrect, as without Profiler SQL Dependencies should be stored in SqlRequestCacheHolder.

(More concerning here is - did any functional tests for sql fail? if not there is a test gap)

nothing has failed. may be there is a gap

cijothomas · 2017-05-04T17:49:51Z

Src/DependencyCollector/Shared/Implementation/ClientServerDependencyTracker.cs

@@ -198,7 +198,7 @@ internal static void AddTupleForSqlDependencies(SqlCommand sqlRequest, Dependenc
            }

            var telemetryTuple = new Tuple<DependencyTelemetry, bool>(telemetry, isCustomCreated);


same comment as above related to SQL without profiler.

removed this check for SQL deps, IsDesktopHttpDiagnosticSourceActivated is only for http

cijothomas · 2017-05-04T17:54:18Z

Src/DependencyCollector/Shared/Implementation/HttpDesktopDiagnosticSourceSubscriber.cs

@@ -48,6 +48,7 @@ public void OnNext(DiagnosticListener value)
                if (value.Name == "System.Net.Http.Desktop")
                {
                    this.sourceSubscription = value.Subscribe(this.parent, (Predicate<string>)null);
+                    DependencyTableStore.Instance.IsDesktopHttpDiagnosticSourceActivated = true;


We are counting on HttpDesktopDiagnosticSourceSubscriber OnNext() to set IsDesktopHttpDiagnosticSourceActivated = true. We definitely need unit test to check this.

Also, is there a guarantee that OnNext will be called before any callbacks to Framework event source?? If not, the IsDesktopHttpDiagnosticSourceActivated will be False, and FrameworkEventSource will start collecting dependencies, which might be later collected by DiagnosticSource. (this may be artificial scenario, but i'd like to know if thats even possible)

Added checks in DependencyTrackingTelemetryModuleTestNet46.

There is no guarantee that subscription will happen before module initialization. It must happen before the first HTTP outgoing call or will never happen at all.
So if there is a HTTP call, in EventSource we will know for sure if diag source is activated or not

cijothomas · 2017-05-04T17:56:17Z

Src/DependencyCollector/Shared/Implementation/HttpDesktopDiagnosticSourceListener.cs

                        var request = (HttpWebRequest)this.requestFetcherRequestEvent.Fetch(value.Value);
-                        this.httpProcessingFramework.OnRequestSend(request);
+                        DependencyCollectorEventSource.Log.BeginCallbackCalled(request.GetHashCode(), value.Key);


Please use a different log message. I understand there is a task opened for fixing the calls backs, but having a new callback log message for 3-4 switch cases in DiagnosticSourceListener should be very easy fix.

Why it is important? I pass name of the callback in the parameter

ok callback name should be sufficient for now.

msftclas added the cla-not-required label Apr 28, 2017

lmolkova requested review from SergeyKanzhelev, cijothomas, Dmitry-Matveev and dnduffy April 28, 2017 01:05

dnduffy reviewed Apr 28, 2017

View reviewed changes

dnduffy approved these changes Apr 28, 2017

View reviewed changes

cijothomas reviewed Apr 28, 2017

View reviewed changes

cijothomas reviewed May 2, 2017

View reviewed changes

cijothomas suggested changes May 2, 2017

View reviewed changes

nizarq reviewed May 2, 2017

View reviewed changes

cijothomas reviewed May 2, 2017

View reviewed changes

lmolkova force-pushed the lmolkova/FixHttpDesktopSource branch from b237375 to 84bd166 Compare May 2, 2017 20:39

cijothomas suggested changes May 3, 2017

View reviewed changes

Liudmila Molkova added 4 commits May 3, 2017 12:04

Track dependency instrumented with HttpDesktop DiagnosticSource in

004ee6f

DiagnosticSource Response event (#509)

remove unused code

7305e11

review: test improvements

b97d60a

review

e233d62

Liudmila Molkova added 4 commits May 3, 2017 12:04

test rename, minor review comments

5e60b26

fix merge issue

886a9e2

Change RDDSource for DiagnosticSource

7d313e5

minor review comments and func tests fix

4f7d151

lmolkova force-pushed the lmolkova/FixHttpDesktopSource branch from e29dc2a to 4f7d151 Compare May 3, 2017 19:05

Add configuration flag to diable Http Desktop DiagnosticSource by def…

0827d68

…ault

SergeyKanzhelev approved these changes May 3, 2017

View reviewed changes

lmolkova mentioned this pull request May 4, 2017

Handle failed dependencies with EventSource and DiagnosticSource #538

Closed

cijothomas suggested changes May 4, 2017

View reviewed changes

up

e973cdd

lmolkova force-pushed the lmolkova/FixHttpDesktopSource branch from db87b9b to e973cdd Compare May 4, 2017 18:35

rename httpDesktopProcessing

5c8a03c

cijothomas approved these changes May 4, 2017

View reviewed changes

cijothomas merged commit f6a6f6f into develop May 4, 2017

cijothomas mentioned this pull request May 10, 2017

Memory leak with MemoryCache #554

Closed

lmolkova mentioned this pull request May 12, 2017

AppId is not parsed with DiagnosticSource HTTP hook on net46 #509

Closed

lmolkova deleted the lmolkova/FixHttpDesktopSource branch November 8, 2017 18:56

		@@ -221,9 +221,21 @@ protected virtual void Dispose(bool disposing)
		/// </summary>
		private void InitializeForFrameworkEventSource()

		@@ -198,7 +198,7 @@ internal static void AddTupleForSqlDependencies(SqlCommand sqlRequest, Dependenc
		}

		var telemetryTuple = new Tuple<DependencyTelemetry, bool>(telemetry, isCustomCreated);

Track dependency instrumented with HttpDesktop DiagnosticSource in Response event (#509) #516

Track dependency instrumented with HttpDesktop DiagnosticSource in Response event (#509) #516

Conversation

lmolkova commented Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnduffy left a comment

Choose a reason for hiding this comment

lmolkova commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas commented Apr 28, 2017

lmolkova commented Apr 28, 2017

lmolkova commented Apr 28, 2017 • edited Loading

lmolkova commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas commented May 2, 2017

nizarq May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova commented May 2, 2017

lmolkova commented May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova commented Apr 28, 2017 •

edited

Loading

lmolkova commented Apr 28, 2017 •

edited

Loading

nizarq May 2, 2017 •

edited

Loading

lmolkova commented May 2, 2017 •

edited

Loading