-
Notifications
You must be signed in to change notification settings - Fork 371
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
Fix telemetry unicode errors #1937
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1937 +/- ##
===========================================
+ Coverage 69.65% 69.66% +0.01%
===========================================
Files 85 85
Lines 11925 11983 +58
Branches 1666 1676 +10
===========================================
+ Hits 8306 8348 +42
- Misses 3251 3263 +12
- Partials 368 372 +4
Continue to review full report at Codecov.
|
tests/protocol/test_wire.py
Outdated
@@ -362,7 +362,7 @@ def test_send_event(self, mock_http_request, *args): | |||
|
|||
event_str = u'a test string' | |||
client = WireProtocol(WIRESERVER_URL).client | |||
client.send_event("foo", event_str) | |||
client.send_event("foo", event_str.encode('utf-8')) |
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.
Should inputs to send_event be of type str, or of type ustr? This maybe harkens back to my prior comment about keeping encode secluded within communicating with the outside world.
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'm assuming that this is, like explained in the other comment, designed to keep changes out of the send_event function.
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.
Technically it should be ustr (which is the same as str for Py3) here and only be encoded in the restutil function which actually sends the data out. But since right now I'm only focusing the changes for the telemetry event, we're encoding the string in this function which is just 1 level up from the restutil function to limit the blast radius of this change.
Does that make sense? We can discuss more on this offline if you have additional doubts.
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.
A couple of small comments
@@ -488,7 +488,7 @@ def put_page_blob(self, url, data): | |||
|
|||
|
|||
def event_param_to_v1(param): | |||
param_format = '<Param Name="{0}" Value={1} T="{2}" />' | |||
param_format = ustr('<Param Name="{0}" Value={1} T="{2}" />') |
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.
can we add utf_8 somewhere in the function name to be more explicit on what it returns ?
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.
Done
6913447
This reverts commit 6d10203.
Description
This PR mainly deals with ensuring that the telemetry events sent by the agent are encoded properly in UTF-8 for both Py2 and Py3. This is to support special characters in telemetry.
Issue #
PR information
Quality of Code and Contribution Guidelines