ref(crons): Include message_type in kafka message #2723
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dav1dde
reviewed
Nov 15, 2023
iker-barriocanal
approved these changes
Nov 16, 2023
relay-server/src/actors/store.rs
Outdated
Comment on lines
1032
to
1039
| /// Discriminate tag used to differentiate these messages from clock pulse messages that are | ||
| /// also placed into the topic. |
There was a problem hiding this comment.
Could we reword this to meet relay coding guidelines (internal)? Thanks!
ensure that there is a precise first line, followed by more information
There was a problem hiding this comment.
I think I fixed this
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-crons-include-message-type-in-kafka-message
branch
from
066b953 to
2795d2a
Compare
There are two types of messages that end up in the ingest-monitors kafka topic, "check_in" (the ones produced here in relay) and "clock_pulse" messages, which are produced externally and are intended to ensure the clock continues to run even when ingestion volume drops. Currently we have some shim logic in our producer which handles adding in this message type when it is missing https://github.com/getsentry/sentry/blob/07522eb8c43828deee6a74de2bb72cf989ec907d/src/sentry/monitors/consumers/monitor_consumer.py#L591-L595 This change introduces the `message_type` key for the check-in messages so we no longer need to shim this.
evanpurkhiser
force-pushed
the
evanpurkhiser/ref-crons-include-message-type-in-kafka-message
branch
from
2795d2a to
e029d69
Compare
|
Gonna get one more look over this just to be safe. |
Dav1dde
approved these changes
Nov 22, 2023
| #[allow(dead_code)] | ||
| #[derive(Debug, Serialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| enum CheckInMessageType { |
There was a problem hiding this comment.
The first sentence of your issue would be nice here. Documenting the use of the values and that relay is only supposed to ever send CheckIn's
There was a problem hiding this comment.
I'll add it in in a diff PR 👍
evanpurkhiser
deleted the
evanpurkhiser/ref-crons-include-message-type-in-kafka-message
branch
jan-auer
added a commit
that referenced
this pull request
Nov 29, 2023
* master: (27 commits) ref(metric-meta): Add metric for total incoming metric meta (#2784) feat(server): Return global config status for downstream requests (#2765) ref(processor): Create event processor sub-module with related code (#2779) fix(metrics): Temporarily restore previous configuration keys for bucket splitting (#2780) feat(metrics): Add source context to code locations (#2781) ref(processor): Split off profile processor code into separate sub-module (#2778) ref(processor): Split off replay processing code into separate sub-module (#2776) ref(metrics): Partition and split metrics buckets just before sending (#2682) feat(spans): Allow well-known path segments in resource URLs (#2770) ref(processor): Move user and client reports processing into separate submodule (#2772) ref(crons): Include message_type in kafka message (#2723) ref(spans): Split tag mapping in specific configs (#2773) release: 23.11.2 feat(metric-meta): Normalize invalid metric names (#2769) feat(spans): Extract main_thread tag for spans (#2761) Add DE Deployments (#2746) ref(processor): Move sessions related code into separate sub-module (#2768) ref(metric-meta): Capture envelope payload in a sentry issue (#2767) release: 0.8.38 ref(normalization): Restore span processing to transactionprocessor (#2764) ...
evanpurkhiser
added a commit
to getsentry/sentry
that referenced
this pull request
Nov 30, 2023
After getsentry/relay#2723 this field is guaranteed to exist in the CheckinMessage. We no longer need to add this in and have it marked as NotRequired.
evanpurkhiser
added a commit
to getsentry/sentry
that referenced
this pull request
Nov 30, 2023
After getsentry/relay#2723 this field is guaranteed to exist in the CheckinMessage. We no longer need to add this in and have it marked as NotRequired.
ArthurKnaus
pushed a commit
to getsentry/sentry
that referenced
this pull request
Dec 1, 2023
After getsentry/relay#2723 this field is guaranteed to exist in the CheckinMessage. We no longer need to add this in and have it marked as NotRequired.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two types of messages that end up in the ingest-monitors kafka topic, "check_in" (the ones produced here in relay) and "clock_pulse" messages, which are produced externally and are intended to ensure the clock continues to run even when ingestion volume drops.
Currently we have some shim logic in our producer which handles adding in this message type when it is missing
https://github.com/getsentry/sentry/blob/07522eb8c43828deee6a74de2bb72cf989ec907d/src/sentry/monitors/consumers/monitor_consumer.py#L591-L595
This change introduces the
message_typekey for the check-in messages so we no longer need to shim this.#skip-changelog