-
Notifications
You must be signed in to change notification settings - Fork 20
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
PCLOUDS-3748: Adjusted align and reduce aggregations #485
PCLOUDS-3748: Adjusted align and reduce aggregations #485
Conversation
|
elif metric_kind.lower().startswith('gauge'): | ||
reducer = 'REDUCE_COUNT_TRUE' if value_type.lower() == 'bool' else 'REDUCE_MEAN' | ||
|
||
return reducer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment about these 2 functions: would you consider creating dictionaries with the aligners and reducers? Then we will separate data from the functions, also you could assign keys like default_aligner, which often are better than comments. "Code never lies, comments sometimes do." 😄
Btw, if you do decide about using the dict, there is this MappingProxyType which is immutable, which may be desired for such a use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with separating data from the functions, but I don't want to maintain keys that I won't use. As we have it now, I don't care for most of the possible values for the types, for instance. And I won't go half way, this is too simple for me to deserve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, valid points 😄
@@ -350,7 +373,7 @@ def create_entity_id(service_name: str, service_dimensions: List[Dimension], tim | |||
return entity_id | |||
|
|||
|
|||
def convert_point_to_ingest_line( | |||
def _convert_point_to_ingest_line( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice of you to include some code cleaning! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do minor things like this in every PR, when we spot them
No description provided.