Analytics Library #53
Conversation
vcai122
requested review from
rm03,
zachHarpaz,
dr-Jess,
minghansun1,
joyliu-q,
Clue88,
dkwncho,
sritanmotati and
nruia-penn
| analytics_recorder, # should be self but consitency | ||
| *entries: FuncEntry | UnaryFuncEntry, | ||
| get_username: Optional[Callable[[FunctionParam, FunctionReturn], Any]] = None, | ||
| ) -> Callable[[Callable[..., Any]], Callable[..., Any]]: |
There was a problem hiding this comment.
Why is ... used along with Any? Isn't ... equivalent to Any?
There was a problem hiding this comment.
the first is the arguments so ... just means any number of arguments, and Any means any return type
analytics/analytics.py
Outdated
| def submit_transaction(self, txn: AnalyticsTxn): | ||
| # TODO: We should make this fail safely, as to not interrupt the everyday | ||
| # business logic products do. | ||
| # However, given that we do not have appropriate logging infrastructure |
There was a problem hiding this comment.
What's the appropriate infra we'd need to change here? Agree on not failing silently rn
There was a problem hiding this comment.
We should be surrounding this with a try catch and logging on failure so we can "silently" fail but be notified its failing.
Think about what we would want if the analytics server went down for example; we wouldn't want all penn labs products routes to start failing because this function fails
analytics/analytics.py
Outdated
| # Offer a 30 second buffer to refresh | ||
| if time.time() >= self.expires_at - 30: | ||
| # TODO: According to @judtinzhang there is a subtle bug here with regards |
There was a problem hiding this comment.
Should probably comment what the bug is so we can be aware of it (if you know what it is)
There was a problem hiding this comment.
No clue. But it's the reason why we are doing the 30 second check. I will make the comment more clear tho
| _refresh_if_outdated() | ||
| self._refresh_expires_at() | ||
| self._refresh_headers() | ||
|
|
||
| self.executor.submit(self.send_message, txn.to_json()) | ||
|
|
||
| def send_message(self, json): | ||
| def _send_message(self, json): |
There was a problem hiding this comment.
_ is convention for private for python
| elif self.get_value is not None: | ||
| ret = self.get_value(func_param, func_return) | ||
| elif self.get_value_use_before is not None: | ||
| ret = self.get_value_use_before(func_param, func_return, before_context) |
There was a problem hiding this comment.
Shouldn't we always enforce that the before context is already computed? For safety
There was a problem hiding this comment.
wdym? it is up to the caller to provide the before_context for this function
if you look at where it is called, you are right that before_context may not be computed in the case that compute_before is not provided, but it just defaults to None in that case. That doesn't seem too bad because if someone is using the before_context by passing get_value_use_before, they should be passing a compute_before anyway
| try: | ||
| return self.filter_do_record(func_param, func_return) | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Just to be clear is False record? Seems to be implied by the above documentation
There was a problem hiding this comment.
You are right False should be do not record as implied by the function being called filter (since for filters True means keep and False means throw away). Good shout!
| if self._compute_filter_do_record(func_param, func_return): | ||
| return { | ||
| "key": key_template.substitute( | ||
| name=f".{self.name}" if self.name else "" |
There was a problem hiding this comment.
Should we fail more loudly here? Empty key is sus
There was a problem hiding this comment.
It's not a failure mode. Users are allowed to not provide a name. Name is just for further customization on the analytics key on the user end, but the library auto generates keys based off of the function name/ route name anyway
No description provided.