Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions src/sentry/api/serializers/models/event.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,8 @@ def serialize(self, obj, attrs, user):
'message': message,
'title': obj.title,
'location': obj.location,
'culprit': obj.culprit,
'user': attrs['user'],
'contexts': attrs['contexts'],
'crashFile': attrs['crash_file'],
'sdk': attrs['sdk'],
# TODO(dcramer): move into contexts['extra']
'context': context,
Expand All @@ -269,11 +267,8 @@ def serialize(self, obj, attrs, user):
'metadata': obj.get_event_metadata(),
'tags': tags,
'platform': obj.platform,
'dateCreated': obj.datetime,
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed a few dependencies on dateCreated

src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx: date={getDynamicText({value: event.dateCreated, fixed: 'Dummy timestamp'})}
src/sentry/static/sentry/app/views/organizationEventsV2/modalLineGraph.jsx: const eventTime = +new Date(currentEvent.dateCreated);
src/sentry/static/sentry/app/views/organizationGroupDetails/eventToolbar.jsx: const dateCreated = moment(evt.dateCreated);

Are these going to be ok if you remove the field? I suspect there is no acceptance test that go through transaction event.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good finds. The modal graph and content component do need to be updated. The GroupDetailsToolbar should be fine as transaction events won't have groups.

'dateReceived': received,
'errors': errors,
'fingerprints': obj.get_hashes(),
'groupingConfig': obj.get_grouping_config(),
'_meta': {
'entries': attrs['_meta']['entries'],
'message': message_meta,
Expand All @@ -285,8 +280,34 @@ def serialize(self, obj, attrs, user):
'tags': tags_meta,
},
}
# Serialize attributes that are specific to different types of events.
if obj.get_event_type() == 'transaction':
d.update(self.__serialize_transaction_attrs(attrs, obj))
else:
d.update(self.__serialize_error_attrs(attrs, obj))
return d

def __serialize_transaction_attrs(self, attrs, obj):
"""
Add attributes that are only present on transaction events.
"""
return {
'startTimestamp': obj.data.get('start_timestamp'),
'endTimestamp': obj.data.get('timestamp'),
}

def __serialize_error_attrs(self, attrs, obj):
"""
Add attributes that are present on error and default event types
"""
return {
'crashFile': attrs['crash_file'],
'culprit': obj.culprit,
'dateCreated': obj.datetime,
'fingerprints': obj.get_hashes(),
'groupingConfig': obj.get_grouping_config(),
}


class DetailedEventSerializer(EventSerializer):
"""
Expand Down
160 changes: 160 additions & 0 deletions src/sentry/data/samples/transaction.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
{
"platform": "python",
"message": "",
"tags": [
[
"application",
"countries"
],
[
"browser",
"Python Requests 2.22"
],
[
"browser.name",
"Python Requests"
],
[
"environment",
"dev"
],
[
"release",
"v0.1"
],
[
"user",
"ip:127.0.0.1"
],
[
"trace",
"a7d67cf796774551a95be6543cacd459"
],
[
"trace.ctx",
"a7d67cf796774551a95be6543cacd459-babaae0d4b7512d9"
],
[
"trace.span",
"babaae0d4b7512d9"
],
[
"transaction",
"/country_by_code/"
],
[
"url",
"http://countries:8010/country_by_code/"
]
],
"breadcrumbs": {
"values": [
{
"category": "query",
"timestamp":1562681591.0,
"message": "SELECT \"countries\".\"id\", \"countries\".\"name\", \"countries\".\"continent\", \"countries\".\"region\", \"countries\".\"surface_area\", \"coun...'CAN'",
"type": "default",
"level": "info"
}
]
},
"contexts": {
"runtime": {
"version": "3.7.3",
"type": "runtime",
"name": "CPython",
"build": "3.7.3 (default, Jun 27 2019, 22:53:21) \n[GCC 8.3.0]"
},
"trace": {
"parent_span_id": "8988cec7cc0779c1",
"type": "trace",
"trace_id": "a7d67cf796774551a95be6543cacd459",
"span_id": "babaae0d4b7512d9"
},
"browser": {
"version": "2.22",
"type": "browser",
"name": "Python Requests"
}
},
"culprit": "/country_by_code/",
"environment": "dev",
"extra": {},
"logger": "",
"metadata": {
"location": "/country_by_code/",
"title": "/country_by_code/"
},
"request": {
"url": "http://countries:8010/country_by_code/",
"headers": [
[
"Accept",
"*/*"
],
[
"Accept-Encoding",
"gzip, deflate"
],
[
"Connection",
"keep-alive"
],
[
"Content-Length",
""
],
[
"Content-Type",
"text/plain"
],
[
"Host",
"countries:8010"
],
[
"Sentry-Trace",
"a7d67cf796774551a95be6543cacd459-8988cec7cc0779c1-1"
],
[
"User-Agent",
"python-requests/2.22.0"
]
],
"env": {
"SERVER_PORT": "8010",
"SERVER_NAME": "a90286977562"
},
"query_string": [
[
"code",
"CAN"
]
],
"method": "GET",
"inferred_content_type": "text/plain"
},
"spans": [
{
"start_timestamp": 1562681591.0,
"same_process_as_parent":true,
"description": "Django: SELECT \"countries\".\"id\", \"countries\".\"name\", \"countries\".\"continent\", \"countries\".\"region\", \"countries\".\"surface_area\", \"coun...'CAN'",
"tags": {
"error":false
},
"timestamp":1562681591.0,
"parent_span_id": "babaae0d4b7512d9",
"trace_id": "a7d67cf796774551a95be6543cacd459",
"op": "db",
"data": {},
"span_id": "b048b4c8fdc5168c"
}
],
"start_timestamp": 1562681591.0,
"timestamp": 1562681591.0,
"transaction": "/country_by_code/",
"type": "transaction",
"user": {
"ip_address": "127.0.0.1"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,10 @@ const EventMetadata = props => {
<MetadataContainer data-test-id="event-id">ID {event.eventID}</MetadataContainer>
<MetadataContainer>
<DateTime
date={getDynamicText({value: event.dateCreated, fixed: 'Dummy timestamp'})}
date={getDynamicText({
value: event.dateCreated || event.endTimestamp * 1000,
fixed: 'Dummy timestamp',
})}
/>
<ExternalLink href={eventJsonUrl} className="json-link">
JSON (<FileSize bytes={event.size} />)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ import {MODAL_QUERY_KEYS, PIN_ICON} from './data';
*/
const getCurrentEventMarker = currentEvent => {
const title = t('Current Event');
const eventTime = +new Date(currentEvent.dateCreated);
const eventTime = +new Date(
currentEvent.dateCreated || currentEvent.endTimestamp * 1000
);

return {
type: 'line',
Expand All @@ -42,7 +44,7 @@ const getCurrentEventMarker = currentEvent => {
},
},
tooltip: {
formatter: ({data}) => {
formatter: () => {
return `<div>${getFormattedDate(eventTime, 'MMM D, YYYY LT')}</div>`;
},
},
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/utils/samples.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def load_data(platform, default=None, sample_name=None):
return

data = CanonicalKeyDict(data)
if platform in ('csp', 'hkpk', 'expectct', 'expectstaple'):
if platform in ('transaction', 'csp', 'hkpk', 'expectct', 'expectstaple'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed ? It was not here in the previous version if I remember correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this the sample transaction event gets data added to the extra, modules and request interfaces which are generally not going to be present on a transaction event payload.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but "transaction" should not be a platform. By doing this we are overloading the meaning and behavior of a parameter. Also you are skipping line 145 where the "platform" field is added to data. Is this intentional.
If you need a different behavior for different event types, I would suggest to pass the event type as an optional parameter, or, better pass a parameter that tells which interfaces to pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

The variable name platform is an awkward name as it is used to locate a sample/fixture file in src/sentry/data/samples. The files there are a mixture of both sample SDK events and special scenarios like empty-exception or python-omittedbody.

return data

data['platform'] = platform
Expand Down
21 changes: 20 additions & 1 deletion tests/sentry/api/serializers/test_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
)
from sentry.models import EventError
from sentry.testutils import TestCase
from sentry.utils.samples import load_data


class EventSerializerTest(TestCase):
Expand All @@ -36,6 +37,8 @@ def test_eventerror(self):
assert 'data' in result['errors'][0]
assert result['errors'][0]['type'] == EventError.INVALID_DATA
assert result['errors'][0]['data'] == {'name': u'ü'}
assert 'startTimestamp' not in result
assert 'timestamp' not in result

def test_hidden_eventerror(self):
event = self.create_event(
Expand Down Expand Up @@ -180,6 +183,22 @@ def test_none_interfaces(self):
assert result['user'] is None
assert result['sdk'] is None
assert result['contexts'] == {}
assert 'startTimestamp' not in result

def test_transaction_event(self):
event_data = load_data('transaction')
event = self.store_event(
data=event_data,
project_id=self.project.id
)
result = serialize(event)
assert isinstance(result['endTimestamp'], float)
assert result['endTimestamp'] == event.data.get('timestamp')
assert isinstance(result['startTimestamp'], float)
assert result['startTimestamp'] == event.data.get('start_timestamp')
assert 'dateCreated' not in result
assert 'crashFile' not in result
assert 'fingerprints' not in result


class SharedEventSerializerTest(TestCase):
Expand All @@ -199,7 +218,7 @@ def test_simple(self):
assert entry['type'] != 'breadcrumbs'


class SnubaEventSerializerTest(TestCase):
class SimpleEventSerializerTest(TestCase):
def test_user(self):
"""
Use the SimpleEventSerializer to serialize an event
Expand Down