Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(eap): Clean up the mutations interface #6344

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

untitaker
Copy link
Member

@untitaker untitaker commented Sep 25, 2024

  • span_id should be a string
  • _sort_timestamp is no longer directly exposed, instead users provide
    start_timestamp_ms and it gets converted using the same logic used in
    the ingestion consumer

This is a follow-up to #6216

* span_id should be a string
* _sort_timestamp is no longer directly exposed, instead users provide
  start_timestamp_ms and it gets converted using the same logic used in
  the ingestion consumer
@ayirr7
Copy link
Member

ayirr7 commented Sep 25, 2024

Thanks for cleaning this up

@ayirr7
Copy link
Member

ayirr7 commented Sep 25, 2024

We need to bump the sentry-kafka-schemas in this too, right?

@untitaker
Copy link
Member Author

untitaker commented Sep 26, 2024

there is some discussion on getsentry/sentry-kafka-schemas#335 that raises doubts about whether it's better to expose _sort_timestamp or start_timestamp_ms. Right now we require all mutations to be sent with _sort_timestamp specified.

IMO we will probably end up with two kinds of mutations in practice:

  1. the ones that react to data on the ingestion topic, or mutations that come from relay or the SDK itself (might not make a difference to the mutations platform, not sure yet)
  2. the ones that query clickhouse periodically and emit mutations based on that. In this case _sort_timestamp can be exposed as an opaque attribute that is forwarded from query results to the mutations interface.

For the first group: _sort_timestamp doesn't exist on the ingestion topic. So either the users of the mutations interface have to calculate _sort_timestamp themselves, or they have to forward their start_timestamp_ms with the assumption baked in that this is somehow indirectly part of the primary key. Either way, some implementation details around _sort_timestamp are going to be leaked.

The other option is that we remove the attribute entirely, and find some other way to fetch this attribute based on some external mapping (organization_id, trace_id, span_id) -> _sort_timestamp from within the mutations consumer. I think that's best long-term but might require additional infra. I wonder if the query layer already has such a mapping.

@phacops
Copy link
Contributor

phacops commented Sep 26, 2024

In either of those cases would I expose start_timestamp. We need to expose and make people use start_timestamp and let _sort_timestamp be the internal timestamp the platform use but not users.

@untitaker untitaker merged commit c810599 into master Oct 10, 2024
30 checks passed
@untitaker untitaker deleted the ref/eap-mutations-interface-cleanup branch October 10, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants