feat: Remove sessions #3271
feat: Remove sessions #3271
Conversation
There was a problem hiding this comment.
@lynnagara thank you for picking this up!
We cannot get rid of sessions entirely, because SDKs still send them. But we can remove the Kafka topic and hard-code the decision to drop sessions after converting them to metrics, essentially everything around:
relay/relay-dynamic-config/src/metrics.rs
Lines 102 to 105 in 0dc102f
| @@ -155,7 +149,6 @@ impl Default for TopicAssignments { | |||
| transactions: "ingest-transactions".to_owned().into(), | |||
| outcomes: "outcomes".to_owned().into(), | |||
| outcomes_billing: None, | |||
| sessions: "ingest-sessions".to_owned().into(), | |||
There was a problem hiding this comment.
Does the ops repo still have configuration for this? We don't want Relay to crash because of an unknown topic parameter.
There was a problem hiding this comment.
Yes. https://github.com/getsentry/ops/blob/master/k8s/clusters/us/default.yaml#L3256-L3257. Though is this comment actually backwards and it needs to be removed from ops first?
relay-server/src/envelope.rs
Outdated
| /// Session update data. | ||
| Session, | ||
| /// Aggregated session data. | ||
| Sessions, |
There was a problem hiding this comment.
Relay now converts session items from SDKs to metrics, but we still need to accept these incoming item types.
lynnagara
force-pushed
the
cleanup-sessions
branch
from
a7a2540
to
24d3f89
Compare
|
thanks for the feedback @jjbayer! i tried to pared this down to the minimum now -- removed the envelope changes and just taking out the part that produces to kafka. I'm not sure why the metrics extraction tests are failing now though - is it possible metrics are not being properly extracted anymore? |
There was a problem hiding this comment.
Though is this comment actually backwards and it needs to be removed from ops first?
I think the comment is mostly about single tenant, are the kafka topics empty there as well? I just checked my local relay and it seems like it's fine with an unknown topic key in the config, so this should be safe to merge.
relay-dynamic-config/src/metrics.rs
Outdated
| pub fn should_drop(&self) -> bool { | ||
| self.drop | ||
| true |
There was a problem hiding this comment.
Personally I would just remove this method.
relay-server/src/services/store.rs
Outdated
| client, | ||
| item, | ||
| )?; | ||
| // Do nothing |
There was a problem hiding this comment.
There's a catch-all at the bottom of this match, so we can just remove this branch.
tests/integration/test_session.py
Outdated
| "sdk": "raven-node/2.6.3", | ||
| } | ||
|
|
||
| sessions_consumer.assert_empty() |
There was a problem hiding this comment.
Shall we just remove the entire test case (same below)?
CHANGELOG.md
Outdated
| - Stop producing to sessions topic, the feature is now fully migrated to metrics ([#3271](https://github.com/getsentry/relay/pull/3271)) | ||
|
|
There was a problem hiding this comment.
| - Stop producing to sessions topic, the feature is now fully migrated to metrics ([#3271](https://github.com/getsentry/relay/pull/3271)) | |
| - Stop producing to sessions topic, the feature is now fully migrated to metrics. ([#3271](https://github.com/getsentry/relay/pull/3271)) |
Makes cardinality limits for Relay more configurable, to be able to test and roll them out more easily. `drop` from the sessions config was removed in getsentry/relay#3271 - required for the librelay update to work
Sessions does not exist anymore