Feature request: captureMethod decorator for sync/async methods

[kennethwkz-mm]

Use case

Important

EDIT: see this comment for updated scope & context

For captureMethod - Method decorator able to support both async/sync function together

Solution/User Experience

We can add new option parameter - async to let user determine when the function need capture sync function.

class Something {
  tracer.captureMethod({ async: false })
  public myMethod {
     // do stuff
  }
}

Alternative solutions

const tracer = new Tracer();

const myFunction = () => { //do something  };

const handler = () => {
  const result = tracer.provider.captureFunc('my subsegment name', myFunction);
}

Acknowledgment

• This feature request meets Lambda Powertools Tenets
• Should this be considered in other Lambda Powertools languages? i.e. Python, Java

[dreamorosi]

Thank you for the feature request.

As discussed on Discord, I think the feature has merit. At the moment the decorator always assumes that the decorated method is async and as such it always returns a Promise and uses AWSXRay.captureAsyncFunc under the hood.

For most cases this is reasonable and/or harmless as it can be awaited by the caller, however I acknowledge that there are other cases in which this is not desirable.

One option to fix this would be the introduction of a parameter where the user specifies whether the decorated function is async or not (like the example in your post).

Another option, that should be investigated, would be to see if there's any way in which the decorator is able to automatically determine whether the method its decorating is sync or async, and based on that adjust its behavior and return type.

[paddymccarroll]

Hi - is this behaviour documented anywhere? If not, should it be added to the docs here? https://docs.powertools.aws.dev/lambda/typescript/main/core/tracer/#methods

[dreamorosi]

Hi @paddymccarroll - sorry for the late reply.

You are right and this is not explicitly documented in the docs, and our API docs actually seem to suggest you can decorate synchronous methods.

We can definitely improve the docs & call out the limitation. Just out of curiosity: did you hit this limitation wanting to decorate a sync method?

[paddymccarroll]

Hi @dreamorosi - no worries, thanks for your reply.

Yep, I had a lot of the methods in my app decorated and only noticed this limitation when the changes were deployed and the app's functionality was broken due to those methods not being awaited. We have the tracing disabled locally, so it's not something I would have noticed there.

[arnabrahman]

@dreamorosi Sorry, somehow I missed the notification. And about your suggestion, I am not sure. I think someone else who has a better understanding of javascript can take a look at this issue. Sorry couldn't be of much help here.

[dreamorosi]

The previous iteration of the discussion around the issue focused on trying to find a way to use the same decorator to handle both synchronous and asynchronous methods.

With that effort not being successful, we should instead consider handling these two cases a separate and provide two decorators: captureMethod and captureMethodSync which can be used respectively on async and synchronous methods.

Ideally the bulk of the implementation for these decorators should be abstracted in a shared function and the each decorator should handle only the different signatures and await-ing of the decorated method.

[shdq]

With that effort not being successful, we should instead consider handling these two cases separately and provide two decorators: captureMethod and captureMethodSync which can be used respectively on async and synchronous methods.

I read the conversation and looked at the decorator code. What if we use originalMethod.constructor.name === 'AsyncFunction' to check the original method and conditionally use captureFunc or captureAsyncFunc to make the decorator handle both sync and async functions? It looks like a quick fix, or am I missing something?

[dreamorosi]

I read the conversation and looked at the decorator code. What if we use originalMethod.constructor.name === 'AsyncFunction' to check the original method and conditionally use captureFunc or captureAsyncFunc to make the decorator handle both sync and async functions? It looks like a quick fix, or am I missing something?

That was my original plan, however I wasn't able to find a way to make it work due to one branch of the function being sync and the other being async. For example, take this function:

const myFunc = async (method: Callable): Promise<X> => {
  if (method.constructor.name === 'AsyncFunction') {
    return await traceAsync()
  } else {
     return trace();
  }
}

Regardless of the branch evaluated at runtime, it will always be treated as asynchronous and return a promise because we are using the async keyword. Conversely, if we don't use the async keyword and instead write it synchronously like this:

const myFunc = (method: Callable): X => {
  if (method.constructor.name === 'AsyncFunction') {
    return traceAsync(); // this is still a pending promise that the caller has to await or .then()
  } else {
     return trace();
  }
}

Additionally, when it comes to typing this function, I didn't find any reliable way of making it apply the correct return type. TypeScript expects any function defined as async to return a Promise<T>, this means that regardless of what we write in it, the return type must be (and is) a promise.

In the past months, when working on the Batch Processing utility, which supports both sync & async record handlers, we ended up with discarding this idea (also because of what explained here) and instead be explicit about the type of operation. In that case we created a BatchProcessor (async) and a BatchProcessorSync.

We adopted async as default because most of modern code written in JavaScript is asynchronous, especially in Lambda & when interacting with AWS APIs. This way, even if this means some duplication on our side, customers immediately know what to expect and the types/implementation is also more straightforward.

[dreamorosi]

After discussing this internally once again we have decided to not move forward with this feature for the time being, and instead improve our documentation to clarify that our decorators are currently compatible with asynchronous class methods only (#1778).

Before leaving this comment I have also discussed this with @shdq - who was looking into this topic - on Discord and we are aligned on the decision.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.