-
Notifications
You must be signed in to change notification settings - Fork 25
fix(provider)!: RaygunMessage object in on_grouping_key method
#118
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
Conversation
python3/raygun4py/raygunmsgs.py
Outdated
|
|
||
| def __init__(self): | ||
| self.occurredOn = datetime.utcnow() | ||
| self.occurredOn = datetime.now(UTC) |
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.
Fixes deprecated utcnow warning
| def __copy__(self): | ||
| new_instance = RaygunMessage() | ||
| new_instance.details = self.details.copy() | ||
| new_instance.occurredOn = self.occurredOn | ||
| return new_instance | ||
|
|
||
| def copy(self): | ||
| return self.__copy__() | ||
|
|
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.
Necessary to avoid modifying the original object in pure functions
| ) | ||
|
|
||
| def _transform_message(self, message): | ||
| message = message.copy() |
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.
_transform_message should be pure and not modify the original message object
python3/raygun4py/raygunprovider.py
Outdated
| if message is not None: | ||
| message = utilities.filter_keys(self.filtered_keys, message) | ||
| message["details"]["groupingKey"] = utilities.execute_grouping_key( | ||
| message.get_details()["groupingKey"] = utilities.execute_grouping_key( |
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.
message should be a RaygunMessage object and not a dict, access properties using accessors.
| return message | ||
|
|
||
| def _post(self, raygunMessage): | ||
| raygunMessage = raygunMessage.copy() |
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.
_post should be a pure function and not modify the original raygunMessage
python3/raygun4py/utilities.py
Outdated
| if isinstance(object, raygunmsgs.RaygunMessage): | ||
| updated_object = object.copy() | ||
| updated_object.__dict__.update(iteration_target) | ||
| return updated_object | ||
|
|
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.
If the original object was a RaygunMessage, then we need to create a copy of the object and apply the changes to the internal __dict__
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.
Pull Request Overview
This PR fixes a bug where the on_grouping_key callback received a raw dictionary instead of a RaygunMessage, and adjusts related methods to treat messages as immutable copies. It also updates tests, adds new setters/getters in RaygunMessage, and refines documentation.
- Refactored
filter_keysto detect and return a modifiedRaygunMessageinstead of plain dict - Changed
RaygunSenderto usecopy(),get_details(),set_details(), andset_error() - Added new getters/setters (
get_error,get_details,set_details,set_error) and corresponding unit tests
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| python3/tests/test_utilities.py | Updated tests to capture and assert the return value of filter_keys. |
| python3/tests/test_raygunprovider.py | Assigned the result of _transform_message to msg in existing tests and added an error‐callback test. |
| python3/tests/test_raygunmsgs.py | Added tests for get_error() and get_details(). |
| python3/raygun4py/utilities.py | Refactored filter_keys to copy the underlying dict and return a RaygunMessage if applicable. |
| python3/raygun4py/raygunprovider.py | Updated _transform_message and _post to use RaygunMessage API methods and make copies. |
| python3/raygun4py/raygunmsgs.py | Made timestamps timezone‐aware and added copy(), getters, and setters for message details and error. |
| README.rst | Improved formatting around RaygunMessage usage in custom grouping docs. |
Comments suppressed due to low confidence (3)
python3/raygun4py/utilities.py:14
- The parameter name
objectshadows a Python built-in and is ambiguous. Consider renaming it toobjortargetfor clarity.
def filter_keys(filtered_keys, object):
python3/tests/test_raygunmsgs.py:162
- We should add unit tests for the new
set_error()andset_details()methods to ensure they correctly update theRaygunMessagestate.
def test_get_error(self):
python3/tests/test_utilities.py:7
- Add a test case where
filter_keysis called on aRaygunMessageinstance to verify it returns aRaygunMessage(not a dict) with filtered fields.
def test_filter_keys(self):
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.
Pull Request Overview
This PR ensures that the on_grouping_key callback always receives a RaygunMessage object (not a raw dict), fixes the key‐filtering bug, updates internal message handling methods, and adds corresponding tests and documentation tweaks.
- filter_keys now detects
RaygunMessage, returns a modified copy, and preserves message methods. - Added
set_details/set_errorand updated_transform_message/_postto useget_details/get_error/set_*APIs. - Expanded unit tests for these changes and improved README code formatting.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python3/tests/test_utilities.py | Updated tests to assign the return of filter_keys and added RaygunMessage case |
| python3/tests/test_raygunprovider.py | Changed grouping key tests to capture and inspect the returned message object |
| python3/tests/test_raygunmsgs.py | Added tests for get_error, get_details, set_error, and set_details |
| python3/raygun4py/utilities.py | filter_keys now supports RaygunMessage inputs and returns a message copy |
| python3/raygun4py/raygunprovider.py | _transform_message/_post updated to use new accessors and copy semantics |
| python3/raygun4py/raygunmsgs.py | Added __copy__, copy, and set_* methods to RaygunMessage |
| README.rst | Improved code references formatting |
Comments suppressed due to low confidence (2)
python3/raygun4py/utilities.py:14
- [nitpick] Consider renaming the parameter 'filtered_keys' to 'keys_to_filter' for clearer intent, since 'filtered_keys' can be misread as keys already filtered.
def filter_keys(filtered_keys, obj):
python3/tests/test_utilities.py:32
- Add tests for nested dictionaries inside
RaygunMessage.detailsto ensurefilter_keysrecurses and filters nested keys correctly.
def test_filter_keys_raygun_message(self):
| details = message.get_details() | ||
| details = utilities.filter_keys(self.filtered_keys, details) |
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.
Pass the details to the filter_keys function instead of passing the whole message object
| Returns: | ||
| dict: The filtered dictionary. | ||
| """ | ||
| iteration_target = dict(obj) |
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.
make a copy to avoid modifying the original object
| if isinstance(object, raygunmsgs.RaygunMessage): | ||
| iteration_target = object.__dict__ |
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.
The filter_keys function shouldn't be used with RaygunMessage directly, instead it should be applied to the details
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.
Changes look good 😃 thanks for adding the additional notes throughout the changes!
Description 📝
Purpose:
In the ticket #96 the customer reported that the object provided to the
on_grouping_keycallback was not aRaygunMessageobject but rather a Dictionary.This was caused by a bug in the
filter_keysmethod as the customer pointed out.Approach:
After some iterations, the
filter_keysfunction has been changed so it does not acceptRaygunMessageobjects, and instead it can only be used with dictionaries (e.g. theRaygunMessage.details). This simplifies the implementation and avoids bugs like the one reported in #96 where theRaygunMessagewas overridden.This ensures that
filter_keysdoesn't break the class definition ofRaygunMessage.Then, the rest of the
_post()method had to be fixed to access thedetailsof theRaygunMessageobject correctly.For that I created methods to
set_detailsandset_error.As well, I created unit tests that check that what the documentation says is correct.
Finally, I also fixed the documentation formatting a bit.
This is potentially a breaking change if customers were accessing the object as a dictionary, as they will have to change the code, so a major version should be released.
While implementing this fix, I also made sure that functions are pure and that they don't modify the provided
RaygunMessage, but rather modify a copy and then return it.Type of change
fix:Bug fix (non-breaking change which fixes an issue)feat:New feature (non-breaking change which adds functionality)chore:Chore task, release or small impact changeci:CI configuration changeRelated issues
Test plan 🧪
Author to check 👓
Reviewer to check ✔️