Skip to content
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

AGENT-65 and AGENT-66 - Broken unit test (json encoding) #160

Merged
merged 9 commits into from Apr 10, 2019
Merged

Conversation

echee2
Copy link
Contributor

@echee2 echee2 commented Apr 4, 2019

  1. Json encoder/decoder issue breaking TestLogFileProcessor.test_log_attributes
  2. _fallback_json_encode missing sort_keys

Error for (1) is

======================================================================
ERROR: test_log_attributes (scalyr_agent.tests.log_processing_test.TestLogFileProcessor)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/echee/Documents/code/scalyr/scalyr-agent-2/scalyr_agent/tests/log_processing_test.py", line 1448, in test_log_attributes
    log_attributes=vals['attributes'])
  File "/Users/echee/Documents/code/scalyr/scalyr-agent-2/scalyr_agent/log_processing.py", line 1330, in __init__
    self.__base_event = Event(thread_id=self.__thread_id, attrs=log_attributes)
  File "/Users/echee/Documents/code/scalyr/scalyr-agent-2/scalyr_agent/scalyr_client.py", line 1217, in __init__
    self.__set_attributes( thread_id, attrs )
  File "/Users/echee/Documents/code/scalyr/scalyr-agent-2/scalyr_agent/scalyr_client.py", line 1250, in __set_attributes
    tmp_buffer.write( scalyr_util.json_encode(attributes) )
  File "/Users/echee/Documents/code/scalyr/scalyr-agent-2/scalyr_agent/util.py", line 88, in json_encode
    return _json_encode( obj, sort_keys=True )
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/__init__.py", line 250, in dumps
    sort_keys=sort_keys, **kw).encode(obj)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 209, in encode
    chunks = list(chunks)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 442, in _iterencode
    o = _default(o)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/encoder.py", line 184, in default
    raise TypeError(repr(o) + " is not JSON serializable")
TypeError: {'host': 'scalyr-1'} is not JSON serializable

ANALYSIS

scalyr_agent.util.json_encode does not know how to encode our custom JsonArray and JsonObject by default, if the underlying lib is json or ujson. (If the underlying lib is the fallback json_lib, we're OK)

Furthermore, if underlying is json, we get the loud TypeError as above. But if underlying is ujson, json_encode() insiduously does not raise exception but encodes incorrectly

STATUS

Per discussion with Steven, I have:

  1. Made ujson complain loudly if you try to make it encode a JsonArray/Object
  2. Modified the failing test to correctly pass native dict instead of JsonObject
  3. Added a check to LogFileProcessor to raise exception if you incorrectly pass in a JsonArray/Object

@echee2 echee2 changed the title Fix AGENT-65 and AGENT-66 WIP: Fix AGENT-65 and AGENT-66 Apr 4, 2019
@echee2
Copy link
Contributor Author

echee2 commented Apr 4, 2019

Need you guys' input on this @czerwingithub @imron . Particularly because the failing test pertains to LogFileProcessor which seems risky?

@echee2
Copy link
Contributor Author

echee2 commented Apr 4, 2019

I talked with Steven. Here's the summary:

  1. JsonObject and JsonArray really aren't necessary and possibly be eliminated entirely
  2. Our fallback lib is only a requirement for parsing our JSON (more permissive, comments etc)
    • but once parsed, it does not need to be returning JsonObject and JsonArray. Investigate returning regular python objects.
  3. This failing unit test can just be changed to supply dict instead of JsonObject. Furthermore the code path to sending over logs should be ensuring that JsonObject / JsonArray are NOT passed.

@echee2 echee2 changed the title WIP: Fix AGENT-65 and AGENT-66 AGENT-65 and AGENT-66 - Broken unit test (json encoding) Apr 4, 2019
@echee2 echee2 requested review from imron and removed request for czerwingithub April 5, 2019 08:09
_json_encode = dumps_no_space
_json_decode = json.loads
return lib_name, json_dumps_custom, json.loads

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone passes in a lib_name value that is not json ujson or json_lib (for whatever reason, maybe a spelling mistake, or maybe a new json library is added), then the function will reach the end and return None, instead of an expected 3-value tuple.

It's not a big issue, because it'll probably never happen and if it does we'll get a compile error, but maybe if we get an unrecognized name then we raise an exception with a message that the json lib_name is unsupported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good suggestion. Change has been made & unit test installed to capture.

@imron imron merged commit 36825f6 into master Apr 10, 2019
@czerwingithub czerwingithub deleted the AGENT-66 branch October 7, 2019 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants