fix: Sort breadcrumbs on timestamp before rendering

[kamilogorek]

Reference: https://forum.sentry.io/t/bad-sorting-of-manual-breadcrumbs/3630

At first, I fixed it in the JS SDK getsentry/sentry-javascript#1308, however, after rethinking it, I believe we should do this here, so it's fixed for every SDK instead.

It's out of order, because timestamp is one of the parameters that can be passed to captureBreadcrumb, yet we push them at the end of the breadcrumbs array as they happen.

[MaxBittker]

Awesome, not sure what percentage of events will be effected, but this seems much more correct.

How well does this deal with undefined timestamps - is there a risk that any of the major sdks are sending differently formed data for this key here? or do we normalize them on the server?

the percy snapshot breadcrumbs have been re-ordered - the exceptions seem to all get moved to the top. is this just a problem with that hardcoded mock data we should fix?

[kamilogorek]

I somehow don't have access to Percy to verify this. I checked components that render breadcrumbs and they don't handle missing data, so I assumed they have to be unified at one point if we already allowed that. We might have to double-check it.

@dcramer can you give me Percy access?

[benvinegar]

.sort needs to return -1, 0, or 1.

[benvinegar]

The results in Percy suggest this isn't sorting correctly.

.sort should return -1, 0, or 1. Is it possible this is the cause?

[kamilogorek]

@MaxBittker about undefined timestamps, it cannot happen, we have a check for this -

sentry/src/sentry/interfaces/breadcrumbs.py

Lines 66 to 75 in 96bc1c6

	def normalize_crumb(cls, crumb):
	ty = crumb.get('type') or 'default'
	ts = parse_timestamp(crumb.get('timestamp'))
	if ts is None:
	raise InterfaceValidationError('Unable to determine timestamp ' 'for crumb')
	rv = {
	'type': ty,
	'timestamp': to_timestamp(ts),
	}

@benvinegar true/false implicitly coerces to 1/0 and we care only about whether we return 1 or not in this case.

The issue, however, is much more complicated than this and took me a while to find the main cause. The first crumb, which is the main exception is simply created not on the same day.

Issue flow:

• we render breadcrumbs based on provided data https://github.com/getsentry/sentry/blob/aedcab8f45161ae9d607da273f1fd44db17ba23e/src/sentry/static/sentry/app/components/events/interfaces/breadcrumbs.jsx#L216
• data is extracted from the event and only then we add something called Virtual Crumb, which is created from the event itself https://github.com/getsentry/sentry/blob/aedcab8f45161ae9d607da273f1fd44db17ba23e/src/sentry/static/sentry/app/components/events/interfaces/breadcrumbs.jsx#L195-L199
• this crumb's date is based on dateCreated attribute https://github.com/getsentry/sentry/blob/aedcab8f45161ae9d607da273f1fd44db17ba23e/src/sentry/static/sentry/app/components/events/interfaces/breadcrumbs.jsx#L121-L126
• our acceptance tests are creating sample event with a fixed timestamp https://github.com/getsentry/sentry/blob/master/tests/acceptance/test_issue_details.py#L23-L35
• however, create_sample_event implementation is overriding every breadcrumb's timestamp based on the current date, but event's date stay the same

  sentry/src/sentry/utils/samples.py

  Lines 176 to 192 in e112b9b

  	start = datetime.utcnow()
  	if timestamp:
  	try:
  	start = datetime.utcfromtimestamp(timestamp)
  	except TypeError:
  	pass
  	# Make breadcrumb timestamps relative to right now so they make sense
  	breadcrumbs = data.get('sentry.interfaces.Breadcrumbs')
  	if breadcrumbs is not None:
  	duration = 1000
  	values = breadcrumbs['values']
  	for value in reversed(values):
  	value['timestamp'] = milliseconds_ago(start, duration)
  	# Every breadcrumb is 1s apart
  	duration += 1000

tl;dr (as I believe it works): created sample event uses fixed timestamp, where breadcrumbs are updated based on the current date (which can be seen on Percy's screenshots)

Also, how about doing this operation in the interface instead of UI?

sentry/src/sentry/interfaces/breadcrumbs.py

Lines 53 to 63 in 96bc1c6

	def to_python(cls, data):
	values = []
	for crumb in data.get('values') or ():
	try:
	values.append(cls.normalize_crumb(crumb))
	except InterfaceValidationError:
	# TODO(dcramer): we dont want to discard the entirety of data
	# when one breadcrumb errors, but it'd be nice if we could still
	# record an error
	continue
	return cls(values=values)

[mitsuhiko]

I'm still super -1 on this. What if we only sort a certain subset of breadcrumbs? JavaScript time is fundamentally nonmonotonic and local so breadcrumbs are likely to be fucked whenever the system time readjusts or we get to a timezone change.

[mitsuhiko]

I really do not want to do this. We could offer it as an option (toggle between order of submission / timestamp) optionally.

[kamilogorek]

I'm totally fine keeping it in dataCallback of JS SDK as well and won't hesitate to close this pr.

[benvinegar]

@benvinegar true/false implicitly coerces to 1/0 and we care only about whether we return 1 or not in this case.

It's not that it coerces to 1/0. My understanding is that 1 and 0 is not enough to produce stable results depending on the browser's internal sorting algorithm, which is not standardized.

More here: https://stackoverflow.com/questions/234683/javascript-array-sort-implementation/234777#234777

For example, this occurs in Chrome right now:

[5, 8, 7, 1, 2, 3, 4, 6, 9, 10, 11, 12, 13].sort(function(a, b) {
    return a > b; // true/false ... 1/0
});
// => [4, 5, 3, 1, 2, 6, 7, 8, 9, 10, 11, 12, 13] // not sorted

[kamilogorek]

Not relevant anymore due to v4 SDK.