Add a context manager object and a method to call said object #357
Conversation
- use `watch` method wrapper - rename 'FeatureFlags' to '_BigBrother' - implement ability to add 'extra_data' - rename payload key to 'scope'
There was a problem hiding this comment.
I am seeing few things that we might need to discuss and define.
First of all, the watch() name, I think it should be renamed to scoped() or something related to "scope" since it's quite clear that it's setting a scope.
Scopes can be nested and the best way to represent them is using dictionaries that are merged, as we do in other libraries (at least in the ruby gem).
That means we have let customer do:
with watch({'user_id': 1}):
with watch({'project_id': 2}):
raise Exception('not found')
And the final scope in the raise would be {'user_id': 1, 'project_id': 2}.
There is another aspect to treat here. Since users can do report_exc_info() several times inside a watch() block we shouldn't clear the scopes when we send data to Rollbar, but always in __exit__()
Other thing to consider is that since scopes out of watch() are empty, an exception caught and reported out of watch() will not be able to send the scopes inside the watch() block. An approach to resolve this would be setting a __rollbar_scopes property in the exception in __exit__() method and we take those scopes in the code that sends the exception to Rollbar. I think we do something similar in the ruby gem, but now I am not sure of this.
And to finalize, we need to discuss how that scope: {extra} schema works. extra is never a good name for something that seems to be part of the first iteration of a feature. I mean, it's not "extra", it's the core part of the functionality.
IMHO we should have a scope dictionary with simple keys/values so we can index that information also in clickhouse easily and we can build a good "scopes" functionality for the customers.
Take into account we already use custom in the API, if we want to have also scope we need to define properly how it looks like, since a bad decision can make complicated to build the system around the data we want, like be able to look for that data in clickhouse.
|
@jondeandres I believe I answered all your questions. Let me know what you think.
How does one do this? I tried to get the exception information in the
Ah. I see. I can change the name of the "extra" key. I kind of threw that in there last minute trying to think of a good key for any value that the user might want to pass in that's not a dictionary. I agree with you that a scope dictionary with simple key/values would help us build good functionality for the customers and thus allowing them to add |
@ajtran I think we need to define first what we want to achieve with this
What I see here is that we are adding a functionality not thinking on what we already have on other libraries and/or how the definition of the API would look like. Have into account that with this we are redefining this https://explorer.docs.rollbar.com/#operation/create-item, so we need to have very clear what kind of schema we want and why. Some questions:
I am not sure this is resolved somewhere, point me to the documentation if we have it defined please. The code changes we need to do are small as you can see, but the structures and API definition we define it's critical. |
|
@jondeandres Thanks for the response! I'll try to answer your questions from easiest to more complicated. Ha.
This was a decision on my part to try and handle the issue Walt mentioned above by making the intended uses of
By this you mean, why is
Maybe, scope isn't the right word here. What we are trying to do with this is allow the user to define a portion of the code they want us to watch and if they report within this context/watch/scope, we will tag the occurrence with this information that they had specified.
The primary use case is for the LaunchDarkly integration and to allow the users to setup the code in a way, so that if an item is reported with in the
Yes, we would like for "scope" to be a first class entity of the occurrence and the product. I can see it being helpful to filter by scope and see the occurrences with in a certain scope. If the scope contains feature flag data, it would be nice to see all the occurrences/item with this feature flag tag. |
|
I've still been concerned about situations where Thoughts on the suggestions here? https://gist.github.com/waltjones/3ffac7e966528c2963739814bb596d01 |
Woahh! Thanks for the suggestion, @waltjones ! I think with some minor adjustments, this would work nicely! I went with the approach to save the tag data on the exception and got some help from Brian. What do you think of the most recent commit to attach the tags to the exception on exit and while reporting the exception info, either send the attached tags if there are any or the tags if there are any and there are no attached tags? |
|
@ajtran I had thought Great news though, as all the other stuff was just working around not having the original exception object. Doesn't this mean the global tags array isn't needed any longer? |
|
@waltjones Ah. I believe it's still needed.. for reporting messages inside the context manger and also for caught exceptions that haven't had a chance to exit yet. |
rollbar/__init__.py
Outdated
| @@ -702,6 +719,12 @@ def _report_exc_info(exc_info, request, extra_data, payload_data, level=None): | |||
| if extra_trace_data and not extra_data: | |||
| data['custom'] = extra_trace_data | |||
|
|
|||
| # if there are feature flags attached to the exception, append that to the feature flags on | |||
| # singleton to create the full feature flags stack. | |||
| feature_flags = _feature_flags + getattr(exc_info[1], '_rollbar_feature_flags', [])[::-1] | |||
There was a problem hiding this comment.
Could be that we have repeated feature flags here? I am thinking if we should use set() for this, but in that case we need to add a FeatureVariation class or similar in order to generate a unique __hash__ since dict is not hashable.
There was a problem hiding this comment.
Hm. I think it's alright to have repeated feature flags here, right? Are you thinking of a case like:
with rollbar.watch_feature_flag('feature-a'):
# do some stuff
with rollbar.watch_feature_flag('feature-b'):
# do some other stuff
with rollbar.watch_feature_flag('feature-a'):
# do some more stuff
where the exception happens in the deepest level of code
rollbar/__init__.py
Outdated
| if hasattr(self._registry, 'tags'): | ||
| return self._registry.tags | ||
|
|
||
| return [] |
There was a problem hiding this comment.
why or when would be the case where _LocalTags is initialized, but does not have a tags attribute in self._registry?
removing this if condition, fails this test https://github.com/rollbar/pyrollbar/blob/master/rollbar/test/test_rollbar.py#L289
with this:
test_report_exception_with_same_exception_as_cause (rollbar.test.test_rollbar.RollbarTest) ... ERROR:rollbar:Exception while reporting exc_info to Rollbar. AttributeError("'thread._local' object has no attribute 'tags'",)
Traceback (most recent call last):
File "/Users/ajtran/Development/tmp/test-launchdarkly/pyrollbar/rollbar/__init__.py", line 439, in report_exc_info
return _report_exc_info(exc_info, request, extra_data, payload_data, level=level)
File "/Users/ajtran/Development/tmp/test-launchdarkly/pyrollbar/rollbar/__init__.py", line 722, in _report_exc_info
tags = _tags.value + getattr(exc_info[1], '_rollbar_tags', [])[::-1]
File "/Users/ajtran/Development/tmp/test-launchdarkly/pyrollbar/rollbar/__init__.py", line 1652, in value
return self._registry.tags
AttributeError: 'thread._local' object has no attribute 'tags'
There was a problem hiding this comment.
I think you should try to get the root exception that is triggering that report and see at which moment that's happening
There was a problem hiding this comment.
Ah. Ok ok. I think I see what is happening right now. For this test test_report_exception_with_same_exception_as_cause, we are running the report_exc_info inside a threading.Thread and when we do that, tags is not set in that thread._local, thus this runtime error when trying to call report_exc_info. I think I might know a way around this.
There was a problem hiding this comment.
I've resolved this by initializing self._registry.tags in the append and value method of _LocalTags.
- remove watch method - utilize feature_flag method that models feature flags as tags
rollbar/__init__.py
Outdated
| @@ -788,6 +837,9 @@ def _report_message(message, level, request, extra_data, payload_data): | |||
| _add_lambda_context_data(data) | |||
| data['server'] = _build_server_data() | |||
|
|
|||
| if _tags.value: | |||
| data['tags'] = _flatten_nested_lists(_tags.value) | |||
There was a problem hiding this comment.
What _flatten_nested_lists() is doing? isn't _tags already a flatten list and not nested?
There was a problem hiding this comment.
no, _tags.value at this point would look something like
[
[
{'key': 'feature_flag.name', 'value': 'feature-1'},
{'key': 'feature_flag.data.feature-1.order', 'value': 0}
],
[
{'key': 'feature_flag.name', 'value': 'feature-2'},
{'key': 'feature_flag.data.feature-2.order', 'value': 1},
{'key': 'feature_flag.data.feature-2.variation', 'value': True}
]
]
where each inner list keeps the tags used to represent a feature flag together. I thought this was necessary because when an exception occurs in the scope of a context manager and we pop and attach the tags to the exception.
There was a problem hiding this comment.
Ok I see. I think we need to change the tag manager so it appends each tag to the list:
class _TagManager(object):
"""
Context manager object that interfaces with the `_tags` stack:
On enter, puts the tag object at top of the stack.
On exit, pops off the top element of the stack.
- If there is an exception, attach the tag object to the exception
for rebuilding of the `_tags` stack before reporting.
"""
def __init__(self, tag):
self.tag = tag
def __enter__(self):
_tags.append(self.tag)
That self.tag should be self.tags and we shouldn't do append() but maybe extend() so _tags is always flatten
There was a problem hiding this comment.
I'm not sure I agree with using extend or keeping _tags always flatten. In the __exit__ method:
def __exit__(self, exc_type, exc_value, traceback):
if exc_value:
if not hasattr(exc_value, '_rollbar_tags'):
exc_value._rollbar_tags = []
exc_value._rollbar_tags.append(self.tag)
_tags.pop()
we pop from _tags whenever we exit the context manager. If this was flattened, we'd have to keep track of the number of times we should pop.
Well, I guess this is something we can do. Would something like this be preferred?
def __exit__(self, exc_type, exc_value, traceback):
if exc_value:
if not hasattr(exc_value, '_rollbar_tags'):
exc_value._rollbar_tags = []
exc_value._rollbar_tags.append(self.tag)
for tag in self.tag:
_tags.pop()
There was a problem hiding this comment.
Oh ok, I see what you mean. You are right although maybe we can:
- rename
_tagsto something different that represents better that they are "context tags" or similar - Add a method in
_LocalTagsthat gives you the flatten tags. That way_tagswill hide the internal structure and I think the code that uses it can be cleaner since will not deal with flattening the list, etc...
What you think?
There was a problem hiding this comment.
Oooo I like that! Let me see if I cam implement it :D
|
@jondeandres I've written the tests and updated the OAS in moxapi here. https://github.com/rollbar/moxapi/pull/546 Let me know what you think! |
ajtran
force-pushed
the
context-manager-ld
branch
from
dc931f6 to
bcece26
Compare
Description of the change
Introduce
watchmethod to wrap the calling of the context manager. The context manager_BigBrotherhandles addingscopeobjects to the stack when the code flow enters the context manager and removing the most recentscopeobject when exiting the context manager without an exception. If there is an exception, the most recentscopeobject will be added to the root level of the raw item payload and be sent under the keyscope.Type of change
Related issues
Checklists
Development
Code review