fix(discover) Add missing event fields#820
Conversation
Now if one of these fields is used in a query against the transactions table, they will get properly NULL'd out instead of being passed through unchanged.
evanh
force-pushed
the
evanh/fix/device_charging_field
branch
from
89013e7 to
9a97ec0
Compare
We have this field in the front end but it wasn't here so it wouldn't return results correctly. Also note that all these device and os fields will error out if transactions are selected until this snuba PR is merged: getsentry/snuba#820
We have this field in the front end but it wasn't here so it wouldn't return results correctly. Also note that all these device and os fields will error out if transactions are selected until this snuba PR is merged: getsentry/snuba#820
When we were checking to see if a column should be promoted, we weren't removing the dots from the column name, which meant we might miss some column conversions
evanh
force-pushed
the
evanh/fix/device_charging_field
branch
from
f522603 to
64d191a
Compare
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
Seems ok, just one last clarification, this will return wrong types till I don't fix the type casting in the tags processor. Are we on the same page on this?
I'd rather have that done first so we are never inconsistent in the response. I will try to do that today, should I not manage to do that I will accept this.
|
@fpacifici Yes I understand that this doesn't fix the incorrect typing issue. Doing that fix first makes sense to me. |
PR #820 uncovered that if we return promoted contexts through the contexts[...] syntax we have an inconsistency between errors and transactions, and this basically breaks discover, since the common columns should behave consistently there. Specifically contexts[device.simulator] would return 'True' from the transactions table and '1' from the events table since the context is promoted in the events table (thus stored in a UInt8) but not in the transactions table where the context is only in the contexts column as a string.
| for idx, condition in enumerate(conditions): | ||
| if is_condition(condition): | ||
| if tuple(condition) == ("type", "!=", "transaction"): | ||
| if tuple(condition) == ("type", "=", "error"): |
There was a problem hiding this comment.
I think there are other event types that need to go to the events table (csp, default, etc.) What's the reason for changing this condition ?
There was a problem hiding this comment.
We would either need cases for csp, security and default types or revert this change.
There was a problem hiding this comment.
I changed this for two reasons:
- The check is broken. We will never match
("type", "!=", "transaction")because the condition sent from sentry looks like[[["isNull",["type"]],"=",1],["type","!=","transaction"]]. - It is useful to have some guarantee which dataset discover queries ultimately run against. Right now this works with transactions but not errors, so having this here actually fixes a number of queries that have
event.type:errorbut are not running against the events dataset.
Also, this is a short circuit check for when people specify a specific event. If they specify an event type not in this condition, it will just continue on using the checks below. We don't need new cases here.
There was a problem hiding this comment.
" this is a short circuit check for when people specify a specific event. If they specify an event type not in this condition, it will just continue on using the checks below"
I don't know whether the intention of this condition is such. There are a few other conditions between this one and the final return EVENTS. That seems fine to me but I'll let @markstory confirm. Anyway, since the condition you mention is actually broken today, I guess this does not matter.
There was a problem hiding this comment.
If this condition is not working, how does the the condition on line 52 work?
There was a problem hiding this comment.
The != condition works differently from the = condition. We don't add the isNull check to plain = conditions.
There was a problem hiding this comment.
ok, this is not going to make things worse or more broken, though I would strongly advise for this logic to be revisited be more robust, either now, based on the old query structure, or after the AST is rolled out.
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
back to you to fix the condition
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
did not see further comments around the condition issue.
Let's ensure it gets fixed in a different PR
4 participants
Now if one of these fields is used in a query against the transactions table,
they will get properly NULL'd out instead of being passed through unchanged.