Adding user_id to metrics #1539
Conversation
cartoframes/utils/metrics.py
Outdated
| @@ -79,11 +80,17 @@ def build_metrics_data(event_name): | |||
| 'runtime_env': get_runtime_env() | |||
| } | |||
|
|
|||
| if not isinstance(extra_metrics_data, dict): | |||
There was a problem hiding this comment.
We need @cmongut @ilbambino for the definition of the metrics structure. Maybe they require "user_id" instead of a field called "extra", so I think we could merge both dictionaries (metrics_data + extra_metrics_data)
There was a problem hiding this comment.
Yes, it isn't defined. But the extra part is almost an error. The just uploaded commit it only adds the extra key if the extra_metrics_data is not a dict.
There was a problem hiding this comment.
I wouldn't add the extra key until it's required from Product, so I think we can remove the extra_metrics_data as != dict and return just {}
cartoframes/utils/metrics.py
Outdated
| @@ -93,7 +100,13 @@ def decorator_func(func): | |||
| @functools.wraps(func) | |||
| def wrapper_func(*args, **kwargs): | |||
| result = func(*args, **kwargs) | |||
| post_metrics(event_name) | |||
|
|
|||
| credentials = get_parameter_from_decorator('credentials', func, *args, **kwargs) | |||
There was a problem hiding this comment.
I would extract this to a function build_extra_metrics_data
cartoframes/utils/utils.py
Outdated
| @@ -334,7 +335,8 @@ def is_uuid(text): | |||
| def get_credentials(credentials=None): | |||
| from ..auth import defaults | |||
| _credentials = credentials or defaults.get_default_credentials() | |||
| check_credentials(_credentials) | |||
| if credentials is not None: | |||
There was a problem hiding this comment.
Why is this added? If credentials is None but you have default_credentials it should call check_credentials too.
There was a problem hiding this comment.
There are some tests that have no credentials configured, and check_credentials raises an exception.
There was a problem hiding this comment.
Aha, in that case, I would try to pass a real Credential instance to those specific tests.
There was a problem hiding this comment.
The failing tests were the Map ones, and the Map class doesn't accept a Credentials parameter, so I've used the set_default_credentials function and created a new unset_default_credentails becuase if not, the test_source_no_credentials test fails because it has the default credentials in the global variables.
cartoframes/utils/utils.py
Outdated
| @@ -524,3 +526,18 @@ def fn(*args, **kw): | |||
| except Exception: | |||
| pass | |||
| return fn | |||
|
|
|||
|
|
|||
| def get_parameter_from_decorator(parameter_name, func, *args, **kwargs): | |||
|
Ready for CR again! |
|
Ready for re-CR! |
|
Adding PEP448! 💪 |
There was a problem hiding this comment.
Great work!! LGTM.
Let's ping @ilbambino, so he can include it in the metrics dashboard.
Issue: https://app.clubhouse.io/cartoteam/story/58519/track-user-in-metrics