Skip to content

Refactor EventSource/EventProvider command scheme so it is more understandable #81987

Open
@davmason

Description

@davmason

Right now the code in EventProvider always sends an Update command to EventSource.

See here:

ControllerCommand command = ControllerCommand.Update;
IDictionary<string, string?>? args = null;
bool skipFinalOnControllerCommand = false;
if (controlCode == Interop.Advapi32.EVENT_CONTROL_CODE_ENABLE_PROVIDER)
{
m_enabled = true;
m_level = setLevel;
m_anyKeywordMask = anyKeyword;
m_allKeywordMask = allKeyword;
List<KeyValuePair<SessionInfo, bool>> sessionsChanged = GetSessions();
// The GetSessions() logic was here to support the idea that different ETW sessions
// could have different user-defined filters. (I believe it is currently broken but that is another matter.)
// However in particular GetSessions() does not support EventPipe, only ETW, which is
// the immediate problem. We work-around establishing the invariant that we always get a
// OnControllerCallback under all circumstances, even if we can't find a delta in the
// ETW logic. This fixes things for the EventPipe case.
//
// All this session based logic should be reviewed and likely removed, but that is a larger
// change that needs more careful staging.
if (sessionsChanged.Count == 0)
sessionsChanged.Add(new KeyValuePair<SessionInfo, bool>(new SessionInfo(0, 0), true));
foreach (KeyValuePair<SessionInfo, bool> session in sessionsChanged)
{
int sessionChanged = session.Key.sessionIdBit;
int etwSessionId = session.Key.etwSessionId;
bool bEnabling = session.Value;
skipFinalOnControllerCommand = true;
args = null; // reinitialize args for every session...
// if we get more than one session changed we have no way
// of knowing which one "filterData" belongs to
if (sessionsChanged.Count > 1)
filterData = null;
// read filter data only when a session is being *added*
if (bEnabling &&
GetDataFromController(etwSessionId, filterData, out command, out byte[]? data, out int keyIndex))
{
args = new Dictionary<string, string?>(4);
// data can be null if the filterArgs had a very large size which failed our sanity check
if (data != null)
{
while (keyIndex < data.Length)
{
int keyEnd = FindNull(data, keyIndex);
int valueIdx = keyEnd + 1;
int valueEnd = FindNull(data, valueIdx);
if (valueEnd < data.Length)
{
string key = System.Text.Encoding.UTF8.GetString(data, keyIndex, keyEnd - keyIndex);
string value = System.Text.Encoding.UTF8.GetString(data, valueIdx, valueEnd - valueIdx);
args[key] = value;
}
keyIndex = valueEnd + 1;
}
}
}
// execute OnControllerCommand once for every session that has changed.
OnControllerCommand(command, args, bEnabling ? sessionChanged : -sessionChanged, etwSessionId);
}

Depending on the values of perEventSourceSessionId or isEnabled it is translated to an Enable or Disable command in EventSource.DoCommand:

bool bSessionEnable = (commandArgs.perEventSourceSessionId >= 0);
if (commandArgs.perEventSourceSessionId == 0 && !commandArgs.enable)
bSessionEnable = false;
if (commandArgs.listener == null)
{
if (!bSessionEnable)
commandArgs.perEventSourceSessionId = -commandArgs.perEventSourceSessionId;
// for "global" enable/disable (passed in with listener == null and
// perEventSourceSessionId == 0) perEventSourceSessionId becomes -1
--commandArgs.perEventSourceSessionId;
}
commandArgs.Command = bSessionEnable ? EventCommand.Enable : EventCommand.Disable;

As far as I can tell there is no reason for this complicated scheme, and we could just send Enable/Disable from EventProvider in the first place. We should investigate and either refactor/simplify the code, or document why it's necessary to do it this way.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area-Tracing-coreclrenhancementProduct code improvement that does NOT require public API changes/additions

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions