Skip to content

Support 30 dimensions per dimension set #80

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 6 commits into from
Jul 29, 2022

Conversation

Himtanaya
Copy link
Contributor

@Himtanaya Himtanaya commented Jun 22, 2022

Issue #, if available:
N/A

Description of changes:

  • Updates max dimension limit per dimension set to 30
  • Throws exception if the limit is exceeded.

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

@heitorlessa
Copy link

Wooow!! I checked EMF Spec and it's still 9 dimensions - is there a doc, What's new associated with this quota increase?

@Himtanaya
Copy link
Contributor Author

Himtanaya commented Jun 22, 2022

Wooow!! I checked EMF Spec and it's still 9 dimensions - is there a doc, What's new associated with this quota increase?

We are still in the process of updating the documentation. As of now we are just getting the client libraries ready for the dimension limit update.

@@ -66,6 +68,10 @@ def put_dimensions(self, dimensions: Dict[str, str]) -> None:
# TODO add ability to define failure strategy
return

if len(dimensions) > MAX_DIMENSION_SET_SIZE:
raise DimensionSetExceededError(

Choose a reason for hiding this comment

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

Suggestion: Can we put this code behind a validateDimensionSet method? That way you can avoid duplication of this check and the exception.

@Himtanaya Himtanaya merged commit bc46f91 into awslabs:master Jul 29, 2022
@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
General
Clarify error message

The error message is inconsistent with the actual validation. The code is
checking the length of keys after they've already been extracted from the
dimension set, but the error message refers to the dimension set size. This
could be confusing for users debugging dimension limit issues.

aws_embedded_metrics/serializers/log_serializer.py [35-38]

 if len(keys) > MAX_DIMENSION_SET_SIZE:
-    err_msg = (f"Maximum number of dimensions per dimension set allowed are {MAX_DIMENSION_SET_SIZE}. "
+    err_msg = (f"Maximum number of dimension keys allowed are {MAX_DIMENSION_SET_SIZE}. "
                f"Account for default dimensions if not using set_dimensions.")
     raise DimensionSetExceededError(err_msg)
  • Apply this suggestion
Suggestion importance[1-10]: 5

__

Why: This suggestion slightly improves the clarity of the error message by explicitly referring to keys instead of the overall dimension set. However, it is a minor stylistic change and does not impact functionality.

Low
Fix grammar in error message

The error message uses incorrect grammar. "Maximum number... are" should be
"Maximum number... is". This makes the error message more professional and
easier to understand.

aws_embedded_metrics/logger/metrics_context.py [60-67]

 @staticmethod
 def validate_dimension_set(dimensions: Dict[str, str]) -> None:
     """
     Validates dimension set length is not more than MAX_DIMENSION_SET_SIZE
     """
     if len(dimensions) > MAX_DIMENSION_SET_SIZE:
         raise DimensionSetExceededError(
-            f"Maximum number of dimensions per dimension set allowed are {MAX_DIMENSION_SET_SIZE}")
+            f"Maximum number of dimensions per dimension set allowed is {MAX_DIMENSION_SET_SIZE}")
  • Apply this suggestion
Suggestion importance[1-10]: 5

__

Why: The suggested change corrects a grammatical issue in the error message, improving professionalism. It is a minor enhancement with limited impact on core functionality.

Low
  • More

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.

4 participants