FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

logging null values in telemetry props

Open daesun-park opened this issue 2 years ago • 0 comments

Description

This PR is a continuation of #9237.Currently, the getValidTelemetryProps() method handles logging in the following way:

        // ensure only valid props get logged, since props of logging error could be in any shape
        if (isTaggedTelemetryPropertyValue(val)) {
            props[key] = {
                value: filterValidTelemetryProps(val.value, key),
                tag: val.tag,
            };
        } else {
            props[key] = filterValidTelemetryProps(val, key);
        }

And the filterValidTelemetryProps() method returns null values as "REDACTED (arbitrary object)".

function filterValidTelemetryProps(x: any, key: string): TelemetryEventPropertyType {
    if (Array.isArray(x) && x.every((val) => isTelemetryEventPropertyValue(val))) {
        return JSON.stringify(x);
    }
    if (isTelemetryEventPropertyValue(x)) {
        return x;
    }
    // We don't support logging arbitrary objects
    console.error(`UnSupported Format of Logging Error Property for key ${key}:`, x);
    return "REDACTED (arbitrary object)";
}

This added change (shown below) is sufficient for handling the simple case for when value === null, but will not support an array which contains a null value (i.e. [true, "string", null]).

    if (x === null) {
        return String(x);
    }

However, if we would like to support the logging of arrays containing null values as well, we could also change the TelemetryEventPropertyType to support null (which has been suggested by @vladsud, and may be introduced in #11337 ).

Reviewer Guidance

Questions

  1. The original pull request #9237 seems to want to address the val === null case, but it seems that support for flat arrays in errorLogging has been added since then. Is this sufficient, or should we make changes such that we log flat arrays containing null values as well (i.e. [true, "string", null])?

daesun-park avatar Aug 04 '22 17:08 daesun-park