Skip to content

Commit

Permalink
[8.0.4xx] Fix serialization of TelemetryEventArgs data packets across…
Browse files Browse the repository at this point in the history
… MSBuild worker nodes (#10464)

* Telemetry strings could be null, so handle that case
  • Loading branch information
baronfel authored Aug 6, 2024
1 parent 0c86109 commit 37eb419
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 8 deletions.
2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<!-- Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the MIT license. See License.txt in the project root for full license information. -->
<Project>
<PropertyGroup>
<VersionPrefix>17.11.3</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind>
<VersionPrefix>17.11.4</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind>
<PackageValidationBaselineVersion>17.10.4</PackageValidationBaselineVersion>
<AssemblyVersion>15.1.0.0</AssemblyVersion>
<PreReleaseVersionLabel>preview</PreReleaseVersionLabel>
Expand Down
1 change: 1 addition & 0 deletions src/Build.UnitTests/BuildEventArgsSerialization_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -873,6 +873,7 @@ public void PropertyInitialValueEventArgs()
e => e.HelpKeyword,
e => e.SenderName);
}

[Fact]
public void ReadingCorruptedStreamThrows()
{
Expand Down
77 changes: 77 additions & 0 deletions src/Framework.UnitTests/CustomEventArgSerialization_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -974,5 +974,82 @@ private static void VerifyTaskFinished(TaskFinishedEventArgs genericEvent, TaskF
newGenericEvent.TaskFile.ShouldBe(genericEvent.TaskFile, StringCompareShould.IgnoreCase); // "Expected TaskFile to Match"
newGenericEvent.TaskName.ShouldBe(genericEvent.TaskName, StringCompareShould.IgnoreCase); // "Expected TaskName to Match"
}


[Fact]
public void TestTelemetryEventArgs_AllProperties()
{
// Test using reasonable values
TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = new Dictionary<string, string> { { "Key", "Value" } } };
genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2);

TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent);

VerifyGenericEventArg(genericEvent, newGenericEvent);
VerifyTelemetryEvent(genericEvent, newGenericEvent);
}

[Fact]
public void TestTelemetryEventArgs_NullProperties()
{
// Test using reasonable values
TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = null };
genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2);

TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent);

// quirk - the properties dict is initialized to an empty dictionary by the default constructor, so it's not _really_ round-trippable.
// so we modify the source event for easier comparison here.
genericEvent.Properties = new Dictionary<string, string>();

VerifyGenericEventArg(genericEvent, newGenericEvent);
VerifyTelemetryEvent(genericEvent, newGenericEvent);
}

[Fact]
public void TestTelemetryEventArgs_NullEventName()
{
// Test using null event name
TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = null, Properties = new Dictionary<string, string> { { "Key", "Value" } } };
genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2);

TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent);

VerifyGenericEventArg(genericEvent, newGenericEvent);
VerifyTelemetryEvent(genericEvent, newGenericEvent);
}

[Fact]
public void TestTelemetryEventArgs_NullPropertyValue()
{
// Test using null property value name
TelemetryEventArgs genericEvent = new TelemetryEventArgs { EventName = "Good", Properties = new Dictionary<string, string> { { "Key", null } } };
genericEvent.BuildEventContext = new BuildEventContext(5, 4, 3, 2);

TelemetryEventArgs newGenericEvent = RoundTrip(genericEvent);

VerifyGenericEventArg(genericEvent, newGenericEvent);
VerifyTelemetryEvent(genericEvent, newGenericEvent);
}

private T RoundTrip<T>(T original)
where T : BuildEventArgs, new()
{
_stream.Position = 0;
original.WriteToStream(_writer);
long streamWriteEndPosition = _stream.Position;

_stream.Position = 0;
var actual = new T();
actual.CreateFromStream(_reader, _eventArgVersion);
_stream.Position.ShouldBe(streamWriteEndPosition); // "Stream End Positions Should Match"
return actual;
}

private static void VerifyTelemetryEvent(TelemetryEventArgs expected, TelemetryEventArgs actual)
{
actual.EventName.ShouldBe(expected.EventName);
actual.Properties.ShouldBe(expected.Properties);
}
}
}
16 changes: 9 additions & 7 deletions src/Framework/TelemetryEventArgs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
using System.IO;
using Microsoft.Build.Shared;

#nullable disable

namespace Microsoft.Build.Framework
{
/// <summary>
Expand All @@ -19,12 +17,12 @@ public sealed class TelemetryEventArgs : BuildEventArgs
/// <summary>
/// Gets or sets the name of the event.
/// </summary>
public string EventName { get; set; }
public string? EventName { get; set; }

/// <summary>
/// Gets or sets a list of properties associated with the event.
/// </summary>
public IDictionary<string, string> Properties { get; set; } = new Dictionary<string, string>();
public IDictionary<string, string?> Properties { get; set; } = new Dictionary<string, string?>();

internal override void WriteToStream(BinaryWriter writer)
{
Expand All @@ -34,13 +32,17 @@ internal override void WriteToStream(BinaryWriter writer)
int count = Properties?.Count ?? 0;
writer.Write7BitEncodedInt(count);

if (Properties == null)
{
return;
}

foreach (var kvp in Properties)
{
writer.Write(kvp.Key);
writer.Write(kvp.Value);
writer.WriteOptionalString(kvp.Value);
}
}

internal override void CreateFromStream(BinaryReader reader, int version)
{
base.CreateFromStream(reader, version);
Expand All @@ -51,7 +53,7 @@ internal override void CreateFromStream(BinaryReader reader, int version)
for (int i = 0; i < count; i++)
{
string key = reader.ReadString();
string value = reader.ReadString();
string? value = reader.ReadOptionalString();
Properties.Add(key, value);
}
}
Expand Down

0 comments on commit 37eb419

Please sign in to comment.