Skip to content
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

chore(metrics): expose MetricsInterface as return type of single metric and further improve API docs #3145

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

dreamorosi
Copy link
Contributor

Summary

Changes

Please provide a summary of what's being changed

This PR started out with the goal of modifying the return type of the Metrics.singleMetric() method to use MetricsInterface instead of Metrics. This small change allows for easier customization of the utility and avoids a cyclical type reference.

While working on this, I realized that the MetricsInterface was out of date and had no documentation whatsoever, which means its API page is empty and there's no contextual documentation in the IDE when customers hover any of the interface or properties.

At that point the goal shifted towards making sure the methods and their API docs in the Metrics class and the MetricsInterface were aligned. In doing so, I also noticed that some of the public methods were somewhat under documented and at least in some cases, there were opportunities to either clarify the purpose of the method or add a code snippet.

This ended becoming a seizable PR that further improved on the work started by @am29d in a previous PR and complemented the API docs added in that PR by expanding some of the descriptions and introducing a bunch of links between methods/sections.

For example, now the API docs for the addMetric() method goes in detail on how to use the method but also links to other relevant parts of the API docs:

image

This work was enabled by @am29d's work on fixing our TypeDoc setup and making sure links are properly rendered & followed. I have applied this type of pattern across the entire Metrics package and also made sure all code samples and wording is consistent across API docs & README.

Finally, while there, I noticed that we had a public method named throwOnEmptyMetrics() which was severely under documented and that didn't do what I'd have expected it to do. I marked it as deprecated in favor of a newly added method setThrowOnEmtpymetrics(enabled: boolean) which is clearer on its purpose and is also aligned with other similar public setter methods in terms of naming.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: closes #3132


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi self-assigned this Oct 2, 2024
@dreamorosi dreamorosi requested review from a team as code owners October 2, 2024 14:31
@boring-cyborg boring-cyborg bot added internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) metrics This item relates to the Metrics Utility tests PRs that add or change tests labels Oct 2, 2024
@pull-request-size pull-request-size bot added the size/XXL PRs with 1K+ LOC, largely documentation related label Oct 2, 2024
@dreamorosi
Copy link
Contributor Author

The 2 accepted issues marked by SonarCloud are related to the deprecated method still being used in the unit tests. For now I marked them as such since these tests will be refactored soon when moving the utility to Vitest, and the method will eventually be removed in the next major version.

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dreamorosi! I just have 2 small comments.

I saw you're changing code in Metrics.ts and types/Metrics.ts and for me looks good. If the tests are green, good to merge.

packages/metrics/src/Metrics.ts Outdated Show resolved Hide resolved
packages/metrics/src/Metrics.ts Outdated Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Oct 3, 2024

@dreamorosi dreamorosi merged commit 686e524 into main Oct 3, 2024
21 checks passed
@dreamorosi dreamorosi deleted the chore/metrics_types branch October 3, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) metrics This item relates to the Metrics Utility size/XXL PRs with 1K+ LOC, largely documentation related tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Change MetricsInterface.singleMetric() to have return type MetricsInterface
2 participants