ref(eap): Update interface of eap mutations

[untitaker]

getsentry/snuba#6344

[github-actions]

versions in use:

The following repositories use one of the schemas you are editing. It is recommended to roll out schema changes in small PRs, meaning that if those used versions lag behind the latest, it is probably best to update those services before rolling out your change.

• getsentry/sentry: pip:sentry-kafka-schemas==0.1.111 (upgrade)
• getsentry/snuba: pip:sentry-kafka-schemas==0.1.113
• getsentry/snuba: rust:sentry-kafka-schemas==0.1.113

latest version: 0.1.113

changes considered breaking

schemas/snuba-eap-mutations.v1.schema.json

• Removed a property _sort_timestamp from .filter, so it is no longer accepted. Maybe use additionalProperties?

  {"path": ".filter", "change": {"PropertyRemove": {"lhs_additional_properties": false, "removed": "_sort_timestamp"}}}
  

benign changes

schemas/snuba-eap-mutations.v1.schema.json

• Added a new property.

  {"path": ".filter", "change": {"PropertyAdd": {"lhs_additional_properties": false, "added": "start_timestamp"}}}
  

⚠️ This PR contains breaking changes. Normally you should avoid that and make
your consumer backwards-compatible (meaning that updated consumers can still
accept old messages). There are a few exceptions:

• If consumers already require these invariants in practice, and you're
  just adjusting the JSON schema to reality, ignore this warning.

• If you know what you are doing, this change could potentially be rolled out
  to producers first, but that's not a flow we support.

[ayirr7]

Curious why we had to make this change?

[untitaker]

not strictly necessary, but now there is just one place that computes _sort_timestamp (the snuba consumers), instead of whichever users of the mutations platform we have. _sort_timestamp is an attribute derived from start_timestamp_ms and IMO should've never been exposed outside of snuba in the first place

[untitaker]

please check https://github.com/getsentry/snuba/pull/6344/files

[ayirr7]

makes sense

[evanh]

I think we need to be a little careful here: _sort_timestamp exists so to abstract which timestamp values we use to sort the data. So it might not always be start timestamp.

[untitaker]

@evanh I think we have to either expose _sort_timestamp or start_timestamp_ms to users of the mutation interface. But there's no rush to make this interface change.

[phacops]

We should only expose start_timestamp to users of the mutation interface and note that it's meant to be a microsecond precision timestamp, not milliseconds. And then, we can round it in the consumer to whatever the _sort_timestamp rounds it to (right now it rounding to the second precision).

We do this to be able to sort spans by start_timestamp and have the appropriate order since some spans can be started in fast succession during the same milliseconds. That's also why we store it in ClickHouse as a DateTime64.

Also, we use a float64 in the span, as a timestamp in seconds with microsecond precision, why not keep that? ClickHouse supports ingestion of this timestamp into a DateTime64.

[untitaker]

this is how start timestamp is represented in a span on kafka: https://github.com/getsentry/snuba/blob/a612e70b017a95969a6f6f21332952cf5d386b21/rust_snuba/src/processors/eap_spans.rs#L300

intended to match that

what is the interface supposed to be for EAP then?

[phacops]

I think we need to be a little careful here: _sort_timestamp exists so to abstract which timestamp values we use to sort the data. So it might not always be start timestamp.

While it's true, I think there's a higher chance we move towards sending incomplete spans from the SDKs and having to update end_timestamp, which makes it not viable for the primary key.

[evanh]

I think we need to be a little careful here: _sort_timestamp exists so to abstract which timestamp values we use to sort the data. So it might not always be start timestamp.