Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce 64KB event payload size limit on EventPipe #50600

Merged
merged 7 commits into from
Apr 5, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/native/eventpipe/ep-buffer-manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,9 @@ ep_buffer_manager_write_event (
// Before we pick a buffer, make sure the event is enabled.
ep_return_false_if_nok (ep_event_is_enabled (ep_event));

// Check that the payload size is less than 64 KB (max size for ETW events)
ep_return_false_if_nok (ep_event_payload_get_size (payload) <= 64 * 1024);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ETW might limit its payloads to 64KB, but does EventPipe need to? EventSource doesn't explicitly prevent you from having payloads this large, e.g., your test.

Copy link
Contributor Author

@sywhang sywhang Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't. But ETW will drop the event if it's greater than 64KB and I'd like to enforce the same behavior here since:

  1. We need some kind of limit regardless. Currently >100KB payloads crash the app. We could enforce a 100KB limit instead of 64KB.
  2. But if we do enforce 100KB limit, that leads to a discrepancy between ETW/EventPipe behavior which we then need to go and figure out how customers can find out whether big events are getting dropped on ETW but not on EventPipe and document why such behavior differences exist.
  3. Another approach is to enforce 1MB payload (which we currently do - just not explicitly but because we just fail to allocate buffers greater than that) and increase the max flushing block size to 1MB but that's a pretty inefficient use of space for sessions listening on much smaller-sized payloads (i.e. counters).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. Let me mull this over, but I think 64KB is fine. Should we remove the DebugBreak in the EP_UNLIKELY macro expansion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually like to keep that. It lets us catch exceptions like this rather than silently failing and us never noticing the failures.

sywhang marked this conversation as resolved.
Show resolved Hide resolved

// Check to see an event thread was specified. If not, then use the current thread.
if (event_thread == NULL)
event_thread = thread;
Expand Down
2 changes: 1 addition & 1 deletion src/native/eventpipe/ep-file.c
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ ep_file_alloc (
instance->stream_writer = stream_writer;
instance->format = format;

instance->event_block = ep_event_block_alloc (100 * 1024, format);
instance->event_block = ep_event_block_alloc (1024 * 1024, format);
sywhang marked this conversation as resolved.
Show resolved Hide resolved
ep_raise_error_if_nok (instance->event_block != NULL);

instance->metadata_block = ep_metadata_block_alloc (100 * 1024);
Expand Down
89 changes: 89 additions & 0 deletions src/tests/tracing/eventpipe/bigevent/bigevent.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics.Tracing;
using System.IO;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using System.Collections.Generic;
using Microsoft.Diagnostics.Tools.RuntimeClient;
using Microsoft.Diagnostics.Tracing;
using Tracing.Tests.Common;
using Microsoft.Diagnostics.Tracing.Parsers.Clr;

namespace Tracing.Tests.BigEventsValidation
{

public sealed class BigEventSource : EventSource
{
private static string bigString = new String('a', 100 * 1024);
sywhang marked this conversation as resolved.
Show resolved Hide resolved
private static string smallString = new String('a', 10);

private BigEventSource()
{
}

public static BigEventSource Log = new BigEventSource();

public void BigEvent()
{
WriteEvent(1, bigString);
}

public void SmallEvent()
{
WriteEvent(2, smallString);
}
}


public class BigEventsValidation
{
public static int Main(string[] args)
{
// This test tries to send a big event (>100KB) and checks that the app does not crash
// See https://github.com/dotnet/runtime/issues/50515 for the regression issue
var providers = new List<Provider>()
{
new Provider("BigEventSource")
};

var configuration = new SessionConfiguration(circularBufferSizeMB: 1024, format: EventPipeSerializationFormat.NetTrace, providers: providers);
return IpcTraceTest.RunAndValidateEventCounts(_expectedEventCounts, _eventGeneratingAction, configuration, _Verify);
}

private static Dictionary<string, ExpectedEventCount> _expectedEventCounts = new Dictionary<string, ExpectedEventCount>()
{
{ "BigEventSource", -1 }
};

private static Action _eventGeneratingAction = () =>
{
// Write 10 big events
for (int i = 0; i < 10; i++)
{
BigEventSource.Log.BigEvent();
}
// Write 10 small events
for (int i = 0; i < 10; i++)
{
BigEventSource.Log.SmallEvent();
}
};

private static Func<EventPipeEventSource, Func<int>> _Verify = (source) =>
{
bool hasSmallEvent = false;
source.Dynamic.All += (TraceEvent data) =>
{
if (data.EventName == "SmallEvent")
{
hasSmallEvent = true;
}
};
return () => hasSmallEvent ? 100 : -1;
};
}
}
15 changes: 15 additions & 0 deletions src/tests/tracing/eventpipe/bigevent/bigevent.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier>
<OutputType>exe</OutputType>
<CLRTestKind>BuildAndRun</CLRTestKind>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<CLRTestPriority>0</CLRTestPriority>
<JitOptimizationSensitive>true</JitOptimizationSensitive>
<UnloadabilityIncompatible>true</UnloadabilityIncompatible>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
<ProjectReference Include="../common/common.csproj" />
</ItemGroup>
</Project>