Skip to content

High Resolution Metrics Support #96

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
merged 13 commits into from
Jan 23, 2023

Conversation

meshwa19
Copy link
Contributor

@meshwa19 meshwa19 commented Jan 4, 2023

Description of changes:

  • Added support for High Resolution Metrics. Customers can specify an optional property - storageResolution of the metrics in putMetric() call. If resolution is not specified, metrics will be emitted with standard resolution.

  • Added UTs and modified integration tests for the changes.

  • Updated README.md and examples folder.

  • Updated the canary. See the image below for memory usage. (Note: We will have to wait until the feature is enabled to get metrics in high resolution for the canary)

Screen Shot 2023-01-10 at 8 12 47 AM

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

@meshwa19 meshwa19 changed the title Initial implementation for supporting High Resolution Metrics High Resolution Metrics Support Jan 5, 2023
@meshwa19 meshwa19 requested a review from rayabagi January 10, 2023 16:15
@meshwa19 meshwa19 requested a review from markkuhn January 11, 2023 22:32
Copy link
Contributor

@rayabagi rayabagi left a comment

Choose a reason for hiding this comment

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

Finished with the review.

@markkuhn markkuhn added the enhancement New feature or request label Jan 13, 2023
@meshwa19 meshwa19 requested a review from markkuhn January 23, 2023 15:46
Copy link
Contributor

@markkuhn markkuhn left a comment

Choose a reason for hiding this comment

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

🚀

@meshwa19 meshwa19 merged commit c7c02c4 into awslabs:master Jan 23, 2023
@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix enum comparison logic

The comparison is using identity check (is not) instead of equality check (!=)
for comparing enum values. This can lead to unexpected behavior when comparing
enum values, as they might be equal in value but not the same object instance.

aws_embedded_metrics/validator.py [94-96]

-if name in metric_name_and_resolution_map and metric_name_and_resolution_map.get(name) is not storage_resolution:
+if name in metric_name_and_resolution_map and metric_name_and_resolution_map.get(name) != storage_resolution:
     raise InvalidMetricError(
         f"Resolution for metrics {name} is already set. A single log event cannot have a metric with two different resolutions.")
  • Apply this suggestion
Suggestion importance[1-10]: 8

__

Why: The suggestion corrects a potentially problematic identity check for enum values by using an equality check, ensuring that two equivalent enum members are recognized as equal, which improves correctness.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants