search

aws-powertools / powertools-lambda-typescript

Powertools is a developer toolkit to implement Serverless best practices and increase developer velocity.
https://docs.powertools.aws.dev/lambda/typescript/latest/
MIT No Attribution
1.56k stars 135 forks source link

Feature request: Change MetricsInterface.singleMetric() to have return type MetricsInterface #3132

Open mikebroberts opened 4 days ago

mikebroberts commented 4 days ago

Use case

Metrics doesn't (currently) have a good way to silence all output during unit tests (it requires configuration changes outside the scope of the unit test code.) To work around this I was going to use a fake implementation of the MetricsInterface interface, and pass that to actual code that generates metrics, instead of passing a reference to the concrete Metrics class. At unit test time I can instead pass a fake implementation of this interface that just silently swallows all requests. The problem with this workaround is that the MetricsInterface.singleMetric() method has to return a concrete implementation of Metrics.

Solution/User Experience

Change MetricsInterface.singleMetric() to return MetricsInterface . I don't think (?) this should break anything, and will allow me to replace the concrete implementation at unit test time.

Alternative solutions

Add functionality to the MetricsOptions interface (used in the Metrics class constructor) to take an option something like "silent" which tells the implementation not to log. Or allow passing a custom output sink. I think these are both better options long term, but my suggestion I think is a one-liner that probably fits better with the design anyway (it's typically best to not have interfaces have types that return concretions).

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

dreamorosi commented 4 days ago

Hi @mikebroberts, thank you for opening the issue.

Before getting into the details of the request, have you seen this section of the docs?

By setting the POWERTOOLS_DEV environment variable to "true", the utility reverts to use the regular console object, which you can then silence with a mock implementation or by passing the --silent flag in Jest or Vitest.

Would this help? Most the customers who use Metrics use this method in their tests

mikebroberts commented 4 days ago

@dreamorosi - yup, I saw that Andrea, thanks. My preference is not to use environment level configuration for unit tests since I might run them from a number of contexts - command line, IDE, etc. And so if I can do everything within the code of the unit test then that to me is better. That way whenever I run the test I get the behavior I'm after.

I do fully admit this is a very small edge case, however I felt that (a) the change is small - if I'm right then just one line, and (b) it means that it's a slightly cleaner design anyway since there's not a circular dependency between the interface and the implementation.

dreamorosi commented 4 days ago

Yea, I'm not against the change in itself, we've done the same for Logger in another issue if I recall correctly.

Regarding the env variables in tests, you can still do it programmatically via setup files or at any point before importing your code, but I'm sure you know this and it's besides the point. Ultimately it's about preference and I respect that.

Before I decide to put this in the backlog, allow me to take a deeper look at the change.

At surface level I'm inclined to agree with your initial assessment, so I don't see issues but I want to be sure it's not causing issues elsewhere.

It's already weekend where I'm based, so I'm going to get back to you here on Monday.

mikebroberts commented 4 days ago

Thanks @dreamorosi - no rush on my side. :) I was just cleaning up a few things on my side and noticed this.

dreamorosi commented 8 hours ago

I have started looking into this and I see no issues with moving forward. I'll open a PR to change this shortly and the change will likely land in the next release (ETA ~next week).