Report errors from build telemetry #4054

pepone · 2024-09-11T15:58:57Z

This PR updates the telemetry reporter to report errors, that are then treated as warning and allow the build to continue:

For example, with a disconnected network:

C:\Users\jose\source\repos\icerpc-csharp\examples\slice\Greeter\Server>dotnet build
Restore complete (0.3s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  Server succeeded with 1 warning(s) (1.4s) → bin\Any CPU\Debug\net8.0\Server.dll
    C:\Users\jose\.nuget\packages\icerpc.slice.tools\0.4.0-preview1\build\IceRpc.Slice.Tools.targets(76,5): warning MSB3073: The command "dotnet IceRpc.BuildTelemetry.Reporter.dll --idl slice --hash=0e7817f6cb2092323a7ee81a42bd5301177621e819f3178a04c4522cd8fae240 --contains-slice1=False --contains-slice2=True --src-file-count=1 --ref-file-count=11" exited with code -532462766.

Build succeeded with 1 warning(s) in 2.1s

or with detailed logging enabled /v:d

C:\Users\jose\source\repos\icerpc-csharp\examples\slice\Greeter\Server>dotnet build /v:d
Restore complete (0.3s)
    Determining projects to restore...
    All projects are up-to-date for restore.
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  Server succeeded with 1 warning(s) (1.4s) → bin\Any CPU\Debug\net8.0\Server.dll
    Unhandled exception. IceRpc.IceRpcException: An IceRpc call failed with error 'IceRpcError'.
     ---> System.Net.Sockets.SocketException (11001): No such host is known.
       at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken)
       at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
       at System.Net.Sockets.Socket.<ConnectAsync>g__WaitForConnectWithCancellation|285_0(AwaitableSocketAsyncEventArgs saea, ValueTask connectTask, CancellationToken cancellationToken)
       at IceRpc.Transports.Tcp.Internal.TcpClientConnection.ConnectAsyncCore(CancellationToken cancellationToken) in C:\Users\jose\source\repos\icerpc-csharp\src\IceRpc\Transports\Tcp\Internal\TcpConnection.cs:line 332
       --- End of inner exception stack trace ---
       at IceRpc.Transports.Tcp.Internal.TcpClientConnection.ConnectAsyncCore(CancellationToken cancellationToken) in C:\Users\jose\source\repos\icerpc-csharp\src\IceRpc\Transports\Tcp\Internal\TcpConnection.cs:line 358
       at IceRpc.Transports.Slic.Internal.SlicConnection.<>c__DisplayClass56_0.<<ConnectAsync>g__PerformConnectAsync|0>d.MoveNext() in C:\Users\jose\source\repos\icerpc-csharp\src\IceRpc\Transports\Slic\Internal\SlicConnection.cs:line 149
    --- End of stack trace from previous location ---
       at IceRpc.Transports.Slic.Internal.SlicConnection.<DisposeAsync>g__PerformDisposeAsync|59_0() in C:\Users\jose\source\repos\icerpc-csharp\src\IceRpc\Transports\Slic\Internal\SlicConnection.cs:line 478
       at Program.<Main>$(String[] args) in C:\Users\jose\source\repos\icerpc-csharp\tools\IceRpc.BuildTelemetry.Reporter\Program.cs:line 93
       at Program.<Main>(String[] args)
    C:\Users\jose\.nuget\packages\icerpc.slice.tools\0.4.0-preview1\build\IceRpc.Slice.Tools.targets(76,5): warning MSB3073: The command "dotnet IceRpc.BuildTelemetry.Reporter.dll --idl slice --hash=0e7817f6cb2092323a7ee81a42bd5301177621e819f3178a04c4522cd8fae240 --contains-slice1=False --contains-slice2=True --src-file-count=1 --ref-file-count=11" exited with code -532462766.

Build succeeded with 1 warning(s) in 2.2s

pepone · 2024-09-11T16:01:44Z

tools/IceRpc.BuildTelemetry.Reporter/Program.cs

-    // Shutdown the connection.
-    await connection.ShutdownAsync(cts.Token);
-}
-catch


Just removed this catch block that was ignoring all exceptions

pepone · 2024-09-11T16:02:52Z

tools/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets

@@ -75,13 +75,11 @@
    </OutputHashTask>

    <!-- Run build telemetry -->
-    <BuildTelemetryTask
+    <Exec


We can use the standard Exec task to call the reporter, no need for a custom task.

pepone · 2024-09-11T16:03:56Z

tools/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets

      Condition="'$(IceRpcBuildTelemetry)' != 'false'"
-      ContinueOnError="true"
-    />
+      ContinueOnError="true" />


Alternatively we can use IgnoreExitCode parameter from Exec task. The difference is that errors would be reported as regular messages instead of warnings.

I think that IgnoreExitCode would be preferable.

Maybe our collective sentiment has changed, but I think that a telemetry failure should have no impact on whether your build succeeds or not; and emitting warnings would still cause a failure if warnings are being treated as errors (something that seems to be at least slightly common).

Presumably ContinueOnError=true means we keep going on error.

Keeping in mind this process (slice compilation) is under our control here.

Maybe I misunderstood, but #4049 made it sound like ContinueOnError was insufficient.
Even though, "keep going on error" is definitely the impression you get from it's name.
(maybe only if you treat warnings as errors? But, to be fair, that is something that many developers do.)

Maybe I misunderstood, but #4049 made it sound like ContinueOnError was insufficient.

That was my impression too when I created the bug. But it is not correct. When you set ContinueOnError warnings doesn't make the build fail even if you set treat warnings as errors.

externl

Looks good to me

InsertCreativityHere

Looks much simpler!

tools/IceRpc.Slice.Tools/IceRpc.Slice.Tools.targets

InsertCreativityHere · 2024-09-11T21:16:03Z

tools/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets

      WorkingDirectory="$(IceRpcBuildTelemetryScriptPath)"
-      CompilationHash="$(Hash)"
-      SourceFileCount="$(FileCount)"
+      Command="dotnet IceRpc.BuildTelemetry.Reporter.dll --idl protobuf --hash=$(Hash) --src-file-count=$(FileCount)"


Same here with no equals after the idl switch.

InsertCreativityHere · 2024-09-11T21:20:56Z

tools/IceRpc.Protobuf.Tools/IceRpc.Protobuf.Tools.targets

      Condition="'$(IceRpcBuildTelemetry)' != 'false'"
-      ContinueOnError="true"
-    />
+      ContinueOnError="true" />


I think that IgnoreExitCode would be preferable.

Maybe our collective sentiment has changed, but I think that a telemetry failure should have no impact on whether your build succeeds or not; and emitting warnings would still cause a failure if warnings are being treated as errors (something that seems to be at least slightly common).

bernardnormier · 2024-09-11T23:07:07Z

tools/IceRpc.BuildTelemetry.Reporter/Program.cs

-try
+// Create a cancellation token source to cancel the telemetry upload if it
+// takes too long.
+using var cts = new CancellationTokenSource(timeout);


I find this code oddly written. We use timeout only here, no need for var timeout.

Agree, I just removed the try/catch would rewrite this in a follow up PR where we split the program in protobuf/slice versions.

bernardnormier · 2024-09-11T23:07:28Z

tools/IceRpc.BuildTelemetry.Reporter/Program.cs

+using var cts = new CancellationTokenSource(timeout);
+
+// Create a client connection
+var clientAuthenticationOptions = new SslClientAuthenticationOptions();


No need for this extra variable.

bernardnormier · 2024-09-11T23:08:03Z

tools/IceRpc.BuildTelemetry.Reporter/Program.cs

+var reporter = new ReporterProxy(connection);
+
+// Parse the IDL
+string? idl = args


From the expression below, doesn't sound like in can be null.

bernardnormier · 2024-09-11T23:14:43Z

tools/IceRpc.BuildTelemetry.Reporter/Program.cs

+    .Skip(1)
+    .FirstOrDefault(), out int referenceFileCountResult) ? referenceFileCountResult : 0;
+
+BuildTelemetry buildTelemetry = idl.ToLower() switch


Given how trivial this program is, I would prefer to have separate versions for IceRpc.Protobuf.Tools and IceRpc.Slice.Tools. For a follow-up PR.

Then no need for an "idl" argument to the program.

Also note that most (maybe all) arguments are IDL-specific. --contains-slice1/--contains-slice2/--ref-file-count is Slice-only, which is not clear with the current setup.

bernardnormier · 2024-09-11T23:18:45Z

tools/IceRpc.BuildTelemetry.Reporter/Program.cs

@@ -2,98 +2,92 @@

 using IceRpc;
 using IceRpc.BuildTelemetry;
+using Microsoft.Extensions.Logging;


Where do we use Logging?

Report errors from build telemetry

dbb7f12

pepone force-pushed the buildtelemetry-simplifications branch from 3308d8c to dbb7f12 Compare September 11, 2024 16:01

pepone commented Sep 11, 2024

View reviewed changes

pepone requested review from externl and bernardnormier September 11, 2024 16:04

externl approved these changes Sep 11, 2024

View reviewed changes

InsertCreativityHere approved these changes Sep 11, 2024

View reviewed changes

bernardnormier approved these changes Sep 11, 2024

View reviewed changes

Fix review comments

03e19f9

pepone merged commit 70b0be5 into icerpc:main Sep 12, 2024
8 checks passed

bernardnormier mentioned this pull request Sep 12, 2024

Build telemetry failure makes build fail #4049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report errors from build telemetry #4054

Report errors from build telemetry #4054

pepone commented Sep 11, 2024 •

edited

Loading

pepone Sep 11, 2024

pepone Sep 11, 2024

pepone Sep 11, 2024

InsertCreativityHere Sep 11, 2024

bernardnormier Sep 11, 2024

InsertCreativityHere Sep 11, 2024 •

edited

Loading

pepone Sep 12, 2024

externl left a comment

InsertCreativityHere left a comment

InsertCreativityHere Sep 11, 2024

InsertCreativityHere Sep 11, 2024

bernardnormier Sep 11, 2024

pepone Sep 12, 2024

bernardnormier Sep 11, 2024

bernardnormier Sep 11, 2024

bernardnormier Sep 11, 2024

bernardnormier Sep 11, 2024

Report errors from build telemetry #4054

Report errors from build telemetry #4054

Conversation

pepone commented Sep 11, 2024 • 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

InsertCreativityHere Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

externl left a comment

Choose a reason for hiding this comment

InsertCreativityHere 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

pepone commented Sep 11, 2024 •

edited

Loading

InsertCreativityHere Sep 11, 2024 •

edited

Loading