Skip to content

Refactored MetricsDefinition to use a MetricBuilder and be based off a super class #149

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

Merged

Conversation

lksfrnz
Copy link

@lksfrnz lksfrnz commented Jun 19, 2024

Refactored the MetricDefinition to inherit a Metric super class which can be inherited by other type of metrics (coming soon). Also refactored MetricDefinition to be immutable and created a MetricDefinitionBuilder which behaves similar to how MetricDefinition used to.

Naming is also handled using a setter instead of on creation for simplicity in upcoming metric changes.

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

@lksfrnz lksfrnz force-pushed the MetricsRefactor branch 2 times, most recently from a63d603 to 6a1c623 Compare June 19, 2024 18:32
@lksfrnz lksfrnz marked this pull request as draft June 19, 2024 18:36
@lksfrnz lksfrnz force-pushed the MetricsRefactor branch 3 times, most recently from e2a28ae to df894a1 Compare June 24, 2024 21:33
@lksfrnz lksfrnz marked this pull request as ready for review June 24, 2024 21:40
@markkuhn markkuhn self-requested a review June 24, 2024 21:54
@markkuhn
Copy link
Contributor

If we want to build the feature first before merging into master, then this PR should point to a feature branch and not master.

@lksfrnz
Copy link
Author

lksfrnz commented Jun 25, 2024

Tested f59577b on the example lambda and metrics were reported as expected.

This commit refactored the previous implementation to now follow the lombok Builder's format as suggested by @markkuhn. However, lombok's @Builder becomes very messy when dealing with inheritance which is why only its format was used here instead of the actual annotation.

@lksfrnz
Copy link
Author

lksfrnz commented Jun 27, 2024

Tested c3fa3e8:

  • The example lambda published metrics as expected
  • All relevant unit tests passed

@lksfrnz lksfrnz changed the base branch from master to histogram_feature_branch June 27, 2024 22:08
@lksfrnz lksfrnz force-pushed the MetricsRefactor branch from 0ce5555 to 59e2593 Compare July 2, 2024 18:37
Copy link
Member

@gordonpn gordonpn left a comment

Choose a reason for hiding this comment

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

LGTM

@giancarlokc giancarlokc merged commit 51e2337 into awslabs:histogram_feature_branch Jul 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants