Inconsistent types for new startOperation function

[pjblakey]

Using Typescript, the new startOperation function (#665 #672) exposes 3 overloads:

export declare function startOperation(context: ISpanContext, name: string): CorrelationContextManager.CorrelationContext | null;
export declare function startOperation(context: azureFunctionsTypes.Context, request: azureFunctionsTypes.HttpRequest): CorrelationContextManager.CorrelationContext | null;
export declare function startOperation(context: IncomingMessage | azureFunctionsTypes.HttpRequest, request?: never): CorrelationContextManager.CorrelationContext | null;

I've come across two type issues here. Firstly azureFunctionsTypes.Context is not compatible with the @azure/functions Context type received in a Function trigger, so has to be asserted as any. Could the @azure/functions type itself not be used here, or at least azureFunctionsTypes.Context be exposed so we can assert it to that? (same for HttpRequest)

Secondly, in the case of a non HTTP trigger function such as a timer trigger, you can pass the context along with a string operationName and it works fine now. However there isn't a correct overload for this - would expect:

function startOperation(context: Context, name: string): CorrelationContextManager.CorrelationContext | null;

These are not biggies but would help improve usability, especially as at first looking at the signatures I wasn't sure if I could use startOperation for a non HTTP trigger, when in fact it does work. Some extra documentation in the README around non HTTP triggers would be good too.

[markwolff]

Thanks for the feedback! These seem like reasonable suggestions -- I'll take a look and ping you in the PR

[markwolff]

Quick update from my end. @pjblakey do you have a repro of the azure functions typings causing you an issue? Here is the sample app I am using and it seems to compile okay. Maybe I am missing a tsconfig field that you have?

import appInsights = require("applicationinsights");
appInsights.setup()
    .setAutoCollectPerformance(false)
    .start();

import axios from "axios";
import type * as azure from "@azure/functions";

/**
 * No changes required to your existing Function logic
 */
const httpTrigger: azure.AzureFunction = async function (context, req) {
    const response = await axios.get("https://httpbin.org/status/200");

    context.res = {
        status: response.status,
        body: response.statusText,
    };
};

// Default export wrapped with Application Insights FaaS context propagation
const wrappedHttpTrigger: azure.AzureFunction = async function contextPropagatingHttpTrigger(context: azure.Context, req) {
    // Start an AI Correlation Context using the provided Function context
    const correlationContext = appInsights.startOperation(context, req) || undefined;

    // Wrap the Function runtime with correlationContext
    return await appInsights.wrapWithCorrelationContext(async () => {
        // Run the Function
        return httpTrigger(context, req);
    }, correlationContext)();
};

export default wrappedHttpTrigger;

[pjblakey]

@markwolff ah yes so this is with strict type checking switched on in tsconfig - I'm seeing this compile error:

And similar for a non http trigger passing context and string:

[markwolff]

I see. The issue here is the operationName arg you are passing should be the AzFunction.HttpRequest you get from the function (I guess args[0] in your case?) and then I compute an operation name for you from the request itself. If you want to specify your own operation name, you should look into one of the other non-AzFunctions overload patterns.

My thinking here was to make this startOperation pattern 1:1 drop-in compatible with AzFunctions, since that is by far the most common use case. The rest of the overload patterns cover non-funtion scenarios.

[pjblakey]

Ah so my understanding PR #672 was adding support for non-http trigger AzFunctions that don't have an HttpRequest, such as timer trigger, so we just pass a string to be used as the operation name instead. But the TS overload for this use case startOperation(AzureFunction.context, string) isn't provided.

This is on top of the separate issue of AzureFunction.Context + @azure/functions.Context types being incompatible with each other. I may be missing something but couldn't you use the official @azure/functions types (rather than making your own) to avoid the incompatibility?

[markwolff]

Could you post a larger version of your first screenshot? The overload 2 error is cut off. I'm unable to get this compile error on my side as well, and so ideally I'd just like to see which parts of the interface aren't compatible. Maybe adding [string]: any wildcard to my own version of Context would help here.

I don't directly use @azure/functions types since they break the compatibility of this SDK for TS2 users.

[pjblakey]

Sure so this is the full error:

No overload matches this call.
  Overload 1 of 3, '(context: ISpanContext, name: string): CorrelationContext | null', gave the following error.
    Argument of type 'Context' is not assignable to parameter of type 'ISpanContext'.
  Overload 2 of 3, '(context: Context, request: HttpRequest): CorrelationContext | null', gave the following error.
    Argument of type 'import("/node_modules/@azure/functions/Interfaces").HttpRequest' is not assignable to parameter of type 'import("/node_modules/applicationinsights/out/Library/Functions").HttpRequest'.
      Types of property 'method' are incompatible.
        Type '"GET" | "POST" | "DELETE" | "HEAD" | "PATCH" | "PUT" | "OPTIONS" | "TRACE" | "CONNECT" | null' is not assignable to type 'string'.
          Type 'null' is not assignable to type 'string'.
  Overload 3 of 3, '(context: HttpRequest | IncomingMessage, request?: undefined): CorrelationContext | null', gave the following error.
    Argument of type 'Context' is not assignable to parameter of type 'HttpRequest | IncomingMessage'.
      Type 'Context' is not assignable to type 'IncomingMessage'.ts(2769)

But without using the @azure/functions package types (or turning off strict checking) I'm not sure this can be resolved - the best would be if you could export your types so we can assert accordingly i.e. startOperation(context as azureFunctionTypes.Context, req as azureFunctionTypes.HttpRequest) rather than having to assert any.

However for the second error (to start an operation with no req object) I think this additional overload is needed:

function startOperation(context: azureFunctionTypes.Context, name: string): CorrelationContextManager.CorrelationContext | null;

[markwolff]

Ok I think I understand the type incompat issue now. I will ship a PR to fix 3 things:

1. Make my HttpRequest nullable
2. Re-export it in case there are other type errors
3. Add a (Functions.Context, string) overload pattern

[pjblakey]

Looks good to me! Thanks @markwolff