-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Remove old telemetry event variants and transformation layer #25179
base: main
Are you sure you want to change the base?
Remove old telemetry event variants and transformation layer #25179
Conversation
crates/collab/src/api/events.rs
Outdated
), | ||
}; | ||
|
||
let mut event_properties = serde_json::to_value(&event.event.event_properties).unwrap(); | ||
if let serde_json::Value::Object(ref mut map) = event_properties { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming events in the old format will get rejected here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JosephTLyons the JSON format we send over the wire can't easily change as we have to update server/client in tandem.
It's totally fine to delete the old events if we don't want to process them anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point, users will be upgraded to a version where all events are generated via event!
and those events no longer interact with the code I deleted in collab, as it was only meant to handle the old historical event format. Do I need to roll back the change that flattens the FlexbileEvent out then? I only removed that as it would be the only variable left in the event enum, but if that changes the json format, I can see why we shouldn't do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so (unfortunately) as otherwise there'll be a hard cut-off date. If you could have collab continue parsing either format that would also work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I rolled back to FlexibleEvent
. The idea of having collab parse the wrapped Event and the event data flattened out is appealing, and then we could just remove the wrapped one later down the line, but not sure how to deal with that now. I think this should be good to land after a few more releases. Really just wanted that migration code out of collab.
e3d1c0f
to
c95a78c
Compare
c95a78c
to
ce9769e
Compare
Sounds good!
…On Thu, Feb 20, 2025 at 8:00 PM, Joseph T. Lyons ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In crates/collab/src/api/events.rs
<#25179 (comment)>:
> - ),
- Event::Action(e) => (
- "Action Invoked".to_string(),
- serde_json::to_value(e).unwrap(),
- ),
- Event::Repl(e) => (
- "Kernel Status Changed".to_string(),
- serde_json::to_value(e).unwrap(),
- ),
- Event::Flexible(e) => (
- e.event_type.clone(),
- serde_json::to_value(&e.event_properties).unwrap(),
- ),
- };
-
+ let mut event_properties = serde_json::to_value(&event.event.event_properties).unwrap();
if let serde_json::Value::Object(ref mut map) = event_properties {
Ok, I rolled back to FlexibleEvent. The idea of having collab parse the
wrapped Event and the event data flattened out is appealing, and then we
could just remove the wrapped one later down the line, but not sure how to
deal with that now. I think this should be good to land after a few more
releases. Really just wanted that migration code out of collab.
—
Reply to this email directly, view it on GitHub
<#25179 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAXAQFJ5BL62US5JHXYXYT2Q2JDDAVCNFSM6AAAAABXOZNRR6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMZRG42DENRRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Now that all events have been migrated to use
telemetry::event!
, we can remove the old event variants. It looks like the last round of event migration code landed in stable inv0.173
(#24102). I don't plan to land this for a few weeks, to give users more time to migrate >=v0.173
.As a side note, I've renamed
AssistantEvent
toAssistantEventData
, as it no longer represents an event itself, but simply data being bundled and injected into an event.Once landed, we will need to deploy a new collab.
Release Notes: