feat: add metrics #102
feat: add metrics #102
Conversation
|
Coverage after merging feature/metrics into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private serviceVariable = 'POWERTOOLS_SERVICE_NAME'; | ||
|
|
||
| public get(name: string): string { | ||
| return process.env[name]?.trim() || ''; |
There was a problem hiding this comment.
I may not have caught usages of this, but why return an empty string instead of undefined? Personally I find undefined clearer than an empty string, but I'm aware that this would lead to a string | undefined in a lot of signatures.
Noticed if (targetService.length > 0) { in Metrics.ts which could be reduced to if (targetService).
There was a problem hiding this comment.
I think this logic was taken from the Logger. I do not have a strong opinion on this, my reasoning was that an environment variable is always a string.
Happy to revisit this!
There was a problem hiding this comment.
I did take this from the logger, But slightly agree with @saragerion that ENV vars should always be strings,
Whatever we decide here I think we should apply it consistently across the pacakges
There was a problem hiding this comment.
@bahrmichael what's your take on this? One of these:
- Keep it as it is;
- Change it as you suggested, for the beta;
- Create a ticket to explore the issue after the beta;
I am in favour of 1 or 3.
There was a problem hiding this comment.
I've raised a placeholder issue #165 for visibility of this across all modules
There was a problem hiding this comment.
Thanks for your PR @alan-churley! 🎉
I left few comments, nothing major but curious to get yours and everyone's perspective on the points raised.
Let me know if you have any question.
|
Coverage after merging feature/metrics into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging feature/metrics into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
great job @alan-churley ! I left a new round of comments - none of them are major or a priority for the beta, but let's keep track of possible improvement points via issues please.
Happy to approve once this is done!
| private serviceVariable = 'POWERTOOLS_SERVICE_NAME'; | ||
|
|
||
| public get(name: string): string { | ||
| return process.env[name]?.trim() || ''; |
There was a problem hiding this comment.
@bahrmichael what's your take on this? One of these:
- Keep it as it is;
- Change it as you suggested, for the beta;
- Create a ticket to explore the issue after the beta;
I am in favour of 1 or 3.
|
Coverage after merging feature/metrics into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging feature/metrics into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new RangeError('The number of metrics recorded must be higher than zero'); | ||
| } | ||
|
|
||
| /* TODO: Potentially a logger.warn users here if default namespace should be used? */ |
There was a problem hiding this comment.
What is the default namespace? "Default"? +1 for adding a console.warn.
Description of your changes
Initial draft of the Metrics module,
It's rough, so feedback is very welcome
How to verify this change
Related issues, RFCs
#25
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.