Tracking logs: Treat null context same as missing context#25006
Closed
kdmccormick wants to merge 1 commit intomasterfrom
Closed
Tracking logs: Treat null context same as missing context#25006kdmccormick wants to merge 1 commit intomasterfrom
kdmccormick wants to merge 1 commit intomasterfrom
Conversation
Should resolve the AttributeError on the line
library_name = segment_context.get('library', ...
that occurs when full_segment_event['context'] == None.
This error is currently the #1 in LMS and is obscuring other
errors.
kdmccormick
force-pushed
the
kdmccormick/segment-content-none
branch
from
e6c40fe to
98232f1
Compare
kdmccormick
commented
Sep 17, 2020
| # Start with the context provided by Segment in the "client" field if it exists | ||
| # We should tightly control which fields actually get included in the event emitted. | ||
| segment_context = full_segment_event.get('context', {}) | ||
| segment_context = full_segment_event.get('context') or {} |
Member
Author
There was a problem hiding this comment.
The original line uses the DICT.get('KEY', {}) pattern to look up a value in a dictionary, falling back to an empty dictionary if the key is missing. The issue with this approach is that if KEY is present in DICT, but has a value of None, then the result will be None instead of {}.
My proposed change is to use DICT.get('KEY') or {}, which will return {} whether KEY is missing or present with a None value.
There are several other places in this function (such as line 126 above) that use the old pattern. Should I change those too?
|
Your PR has finished running tests. There were no failures. |
Member
Author
|
This error does not seem to be occurring any more; closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Should resolve the AttributeError on the line:
that occurs when
full_segment_event['context'] == None.This error is currently the #1 in LMS and is obscuring other errors.
I think this was the cause of the alert this morning; my NewRelic-fu isn't developed enough to be sure though.
I don't know whether this is the right fix. Maybe it's good to have that AttributeError yelling at us until we can find out why we are getting segment events with
Noneas their context. I'm posting in hope of soliciting feedback.What do you think @stvstnfrd ?Update: This can't be the cause of the alert this morning, because it's owned by
platform-data-de, and we still see the error spike even when filtering that DE's endpoints out of the query.