ApplicationInsights-node.js icon indicating copy to clipboard operation
ApplicationInsights-node.js copied to clipboard

Provide better TypeScript definition for `addTelemetryProcessor` callback.

Open NoelAbrahams opened this issue 4 years ago • 2 comments

In the following the TypeScript definition should be set up as a union type with baseType being the discriminant so that it narrows correctly in the if statement:

import * as ai from 'applicationinsights';

ai.defaultClient.addTelemetryProcessor((envelope, _context) => {

if (envelope.data.baseType === 'RemoteDependencyData') {

  envelope.data     // Problem here
}

Expected The type envelope.data should narrow to ai.Contracts.Data<ai.Contracts.RemoteDependencyData>.

Actual The type is ai.Contracts.Base

I suggest this is important because the current workaround feels like we are dealing with AI internals that may break in future versions.

Thanks, and great work by the way. Noel

NoelAbrahams avatar Mar 20 '20 08:03 NoelAbrahams

Updating the contracts would be a breaking change and will require a major version update, I'm trying to figure out what would be the best strategy here without breaking anyone, we also have a mix of interfaces and classes inside contracts, ideally we only expose interfaces so customers don't reference the actual implementation directly.

Have you tried something like this to unblock you

AppInsights.defaultClient.addTelemetryProcessor((envelope: Envelope, context: Context) => { if (envelope.data.baseType === 'RemoteDependencyData') { let data = envelope.data as Data<RemoteDependencyData>; data.baseData.success = false; } })

hectorhdzg avatar Dec 11 '20 23:12 hectorhdzg

@hectorhdzg

We are doing something similar:

type EnvelopeType = 'EventData' | 'RequestData' | 'RemoteDependencyData';

ai.defaultClient.addTelemetryProcessor((envelope, _context) => {

    if ((envelope?.data?.baseType as /*HERE*/ EnvelopeType) === 'RemoteDependencyData') {

        const base = envelope.data as /*HERE*/ ai.Contracts.Data<Partial<ai.Contracts.RemoteDependencyData>>;
        const request = base.baseData;

        if (request) {

            // Filter out CosmosDB since it has a custom handler 
            if (request.target?.includes('cosmosDBDomain')) {
                return false;
            }

        }
    }
    return true;
});

This code can break any time CosmosDB internals change. The code should not have any as type assertions, which I've highlighted with the /*HERE*/ comment. The as assertions are simply guessing your intentions, which we discovered by setting breakpoints and seeing what is happening at run time. If you decide to change your internal behaviour at any time, then this code will silently break because we are telling TypeScript to force an assertion via as.

I hope I've explained the danger of the situation clearly enough. This is probably worth a breaking change to fix, if that is necessary.

NoelAbrahams avatar Dec 12 '20 09:12 NoelAbrahams