Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
1 of 2 tasks
mikebroberts opened this issue Sep 27, 2024 · 7 comments · Fixed by #3145
Closed
1 of 2 tasks
Assignees
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility metrics This item relates to the Metrics Utility

Comments

@mikebroberts
Copy link

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.

@mikebroberts mikebroberts added feature-request This item refers to a feature request for an existing or new utility triage This item has not been triaged by a maintainer, please wait labels Sep 27, 2024
@dreamorosi
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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.

@dreamorosi dreamorosi added metrics This item relates to the Metrics Utility discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait labels Sep 27, 2024
@dreamorosi dreamorosi self-assigned this Sep 27, 2024
@mikebroberts
Copy link
Author

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

@dreamorosi dreamorosi moved this from Ideas to Backlog in Powertools for AWS Lambda (TypeScript) Oct 1, 2024
@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined labels Oct 1, 2024
@dreamorosi dreamorosi moved this from Backlog to Working on it in Powertools for AWS Lambda (TypeScript) Oct 1, 2024
@dreamorosi
Copy link
Contributor

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).

Copy link
Contributor

github-actions bot commented Oct 3, 2024

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments 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 github-actions bot added pending-release This item has been merged and will be released soon and removed confirmed The scope is clear, ready for implementation labels Oct 3, 2024
Copy link
Contributor

github-actions bot commented Oct 8, 2024

This is now released under v2.9.0 version!

@github-actions github-actions bot added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon labels Oct 8, 2024
@dreamorosi dreamorosi moved this from Coming soon to Shipped in Powertools for AWS Lambda (TypeScript) Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility metrics This item relates to the Metrics Utility
Projects
2 participants