Modify all existing Telemetry types to implement IExtension #890

cijothomas · 2018-08-07T23:10:34Z

Following up on this PR (#875) which introduced the notion of IExtension ( #871) which enabled strongly typed extensions to existing types.

This PR makes all existing types (like RequestTelemetry, DependencyTelemetry etc) implement IExtension , thereby emitting serialization information. Current serialization logic is replaced with this one. With this, customers using non-AI backend/channel can use the existing types and serialize them to any format (bond,xml etc).

Note: Few items like updating public API in api analyzer is pending.

I ran Unit Tests locally.

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

Design discussion issue #
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.

…ialization module does that.

… for ISerialziationWriter

…r now.

SergeyKanzhelev · 2018-08-08T20:27:50Z

src/Microsoft.ApplicationInsights/DataContracts/AvailabilityTelemetry.cs

+        /// <summary>
+        /// Deeply clones the Extension of <see cref="AvailabilityTelemetry"/> object.
+        /// </summary>
+        IExtension IExtension.DeepClone()


this is not ideal. DeepClone needs to return ITelemetry so you can pass it to the processors and initializers. So maybe ITelemetry should be implementing IExtension?

SergeyKanzhelev · 2018-08-08T20:30:00Z

src/Microsoft.ApplicationInsights/DataContracts/AvailabilityTelemetry.cs

+        {            
+            serializationWriter.WriteProperty("name", this.WriteTelemetryName(TelemetryName));
+            this.WriteEnvelopeProperties(serializationWriter);
+            Utils.CopyDictionary(this.Context.GlobalProperties, this.Data.properties);


serialize shouldn't mutate an object. Move dictionary copying some other place

Addressed this.

SergeyKanzhelev · 2018-08-08T20:30:53Z

src/Microsoft.ApplicationInsights/DataContracts/AvailabilityTelemetry.cs

+        /// <inheritdoc/>
+        public void Serialize(ISerializationWriter serializationWriter)
+        {            
+            serializationWriter.WriteProperty("name", this.WriteTelemetryName(TelemetryName));


if AvailabilityTelemetry responsible for the entire envelope - it also need to serialize things like flags and samplingPercentage.

Its doing it next line.

SergeyKanzhelev

See comments for AvailabilityTelemetry

IExtension for extensions. ISerializableWithWriter for classes emitting serialization info.

…ializableWithWriter, ITelemetry dont implement the interface to avoid conufsion with the IExtension in ITelemetry

…ize method to follow the practise of NOT modifying data within serialize method.

SergeyKanzhelev · 2018-08-08T23:21:14Z

src/Microsoft.ApplicationInsights/Extensibility/IExtension.cs

@@ -3,16 +3,7 @@
    /// <summary>
    /// Interface for defining strongly typed extensions to telemetry types.
    /// </summary>
-    public interface IExtension
+    public interface IExtension : ISerializableWithWriter, ICloneable


do we still need ISerializableWithWriter as a separate interface?

Its now used by all the InternalData classes. The main classes like RequestTelemetry invokes serialize on the InternalData..

SergeyKanzhelev · 2018-08-08T23:23:12Z

src/Microsoft.ApplicationInsights/Microsoft.ApplicationInsights.csproj

@@ -52,9 +52,11 @@
    <PackageReference Include="MicroBuild.Core" Version="0.3.0">
      <PrivateAssets>All</PrivateAssets>
    </PackageReference>
+    <!--
    <PackageReference Include="PublicApiAnalyzer" Version="1.0.0-alpha001">


unintended?

Will enable it back once other comments are addressed. I chose to modify public.shipped.txt only after everything else is ready

I start review by looking at these files. Especially when there are so many files in PR. I'd appreciate if you can keep updating API files

Understood. I have enabled it back and modified all public api files to reflect correct APIs. ( I disabled it because it was a disturbance while i was still tweaking API!)

SergeyKanzhelev · 2018-08-08T23:24:25Z

...nsights/Extensibility/Implementation/External/RemoteDependencyDataISerializableWithWriter.cs

+    /// <summary>
+    /// Partial class to implement ISerializableWithWriter
+    /// </summary>
+    internal partial class RemoteDependencyData : ISerializableWithWriter


perhaps Sanitize can be moved to this interface as well at some point

SergeyKanzhelev · 2018-08-08T23:26:17Z

src/Microsoft.ApplicationInsights/Extensibility/ICloneable.cs

+    /// <summary>
+    /// Interface for defining method to Deeply clone the members.
+    /// </summary>
+    public interface ICloneable


The only consumer of this interface is IExtension. Why we need it as an interface then? Just define a method on that interface

Right, removed.

SergeyKanzhelev

Few more comments. Overall looks better

…ing before calling seiralize

cijothomas · 2018-08-09T01:00:57Z

@SergeyKanzhelev Pubic API files added back. Please continue reviewing.

SergeyKanzhelev · 2018-08-09T18:17:28Z

src/Microsoft.ApplicationInsights/DataContracts/AvailabilityTelemetry.cs

@@ -178,6 +178,18 @@ public ITelemetry DeepClone()
            return new AvailabilityTelemetry(this);
        }

+        /// <inheritdoc/>
+        public void Serialize(ISerializationWriter serializationWriter)


Everything looks better now.... Now talk about usage of it - is it what our partners expect. I wonder if we need to have separate methods for envelope and separate for data part of it. This way envelope structure is not forced on consumer and there is a choice to optimize an envelope structure for specific channels. What do you think?

SergeyKanzhelev

Will you switch to use these serializes instead of current one in a separate PR?

cijothomas added 19 commits July 31, 2018 14:05

Modify DependencyTelemetry to implement IExtension

8f8f57c

Trials

ae9ec08

more

522754e

al telemetry now does time and name as per old formats

3057f67

Check for null severity level. Added StartObject for every Type

f6ab449

removed temp things.

f0d70a2

exception handle severity level correctly

805468b

Individual Telemetry implementations need not call start stop but ser…

4bed401

…ialization module does that.

Serialization to completely dependend on Serialize method of IExtension

08ed95a

Supportwriting complex combject in ISerializationWriter

c513199

Make base data implement IExtension and use WriteComplex Property API…

e1bf746

… for ISerialziationWriter

Removed dead code. entire serialization occurs by ISerializationWrite…

a38871d

…r now.

Dependency - duration serializes to default value when not set

66c981d

minor

b917369

Unit test the JsonSerializationWriter by serializing a complex object

719fbbf

Serialize duration correctly

d3ad81b

Reuse code to write envelop properties

17f8ee0

Renamings and testing jsonserializationwriter

f9000fc

Merge branch 'develop' into cithomas/extensibiltychanges

da90aec

SergeyKanzhelev reviewed Aug 8, 2018

View reviewed changes

SergeyKanzhelev suggested changes Aug 8, 2018

View reviewed changes

cijothomas added 4 commits August 8, 2018 15:27

Modified interfaces to be more correct.

5b7fb36

IExtension for extensions. ISerializableWithWriter for classes emitting serialization info.

ITelemetry contains a Serialize method. Though same signature as ISer…

2fec31e

…ializableWithWriter, ITelemetry dont implement the interface to avoid conufsion with the IExtension in ITelemetry

Copying TC.GlobalProperties to Item.Properties is done outside serial…

1f19af8

…ize method to follow the practise of NOT modifying data within serialize method.

Fix typos comments

360931b

SergeyKanzhelev reviewed Aug 8, 2018

View reviewed changes

SergeyKanzhelev suggested changes Aug 8, 2018

View reviewed changes

cijothomas added 6 commits August 8, 2018 16:28

ITelemetry has serialize so JsonSerializer dont need to cast to anyth…

f2a412a

…ing before calling seiralize

Removed ICloneable as clone can be defined in IExtension itelf.

9a8bd60

All types to serialzie their Extension

c84fdc1

Re-enable public api analyzer

f2b6936

Public API files added

74b83e1

remove bad comment

95f3307

cijothomas added 2 commits August 8, 2018 20:04

Test fix

35197ed

Test fix

f7f98d7

SergeyKanzhelev reviewed Aug 9, 2018

View reviewed changes

SergeyKanzhelev approved these changes Aug 9, 2018

View reviewed changes

cijothomas merged commit 39518a7 into develop Aug 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify all existing Telemetry types to implement IExtension #890

Modify all existing Telemetry types to implement IExtension #890

cijothomas commented Aug 7, 2018 •

edited

Loading

SergeyKanzhelev Aug 8, 2018

SergeyKanzhelev Aug 8, 2018

cijothomas Aug 8, 2018

cijothomas Aug 8, 2018

SergeyKanzhelev Aug 8, 2018

cijothomas Aug 8, 2018

SergeyKanzhelev left a comment

SergeyKanzhelev Aug 8, 2018

cijothomas Aug 9, 2018

SergeyKanzhelev Aug 8, 2018

cijothomas Aug 8, 2018

SergeyKanzhelev Aug 8, 2018

cijothomas Aug 9, 2018

SergeyKanzhelev Aug 8, 2018

SergeyKanzhelev Aug 8, 2018

cijothomas Aug 8, 2018

SergeyKanzhelev left a comment

cijothomas commented Aug 9, 2018

SergeyKanzhelev Aug 9, 2018

SergeyKanzhelev left a comment

Modify all existing Telemetry types to implement IExtension #890

Modify all existing Telemetry types to implement IExtension #890

Conversation

cijothomas commented Aug 7, 2018 • 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

Choose a reason for hiding this comment

SergeyKanzhelev 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

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

cijothomas commented Aug 9, 2018

Choose a reason for hiding this comment

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

cijothomas commented Aug 7, 2018 •

edited

Loading