-
Notifications
You must be signed in to change notification settings - Fork 10
feat: view SDK events on ldcli dev server #596
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
Conversation
internal/dev_server/sdk/routes.go
Outdated
| router.HandleFunc("/bulk", DevNull) | ||
| router.HandleFunc("/bulk", SdkEventsReceiveHandler) | ||
| router.HandleFunc("/diagnostic", DevNull) | ||
| router.HandleFunc("/events/tee", SdkEventsTeeHandler) |
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.
What is this for?
internal/dev_server/sdk/routes.go
Outdated
| router.HandleFunc("/bulk", SdkEventsReceiveHandler) | ||
| router.HandleFunc("/diagnostic", DevNull) | ||
| router.HandleFunc("/events/tee", SdkEventsTeeHandler) | ||
| router.Handle("/events/bulk/{envId}", EventsCorsHeaders(DevNull)) |
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.
This one is for js sdk events
| router.HandleFunc("/mobile", DevNull) | ||
| router.HandleFunc("/mobile/events", DevNull) |
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.
These are for old mobile SDKs
|
|
||
| const newEvent: EventData = { | ||
| id: Math.random().toString(36).slice(2, 11), | ||
| timestamp: Date.now(), |
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.
😕 what is this doing?
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.
@moribellamy setting an initial fake event like this necessary still or was this just during prototyping?
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.
this looks like a real event to me, as collected by the server on line 25. id is so dom elements can have something unique to help react caching.
am i missing something?
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.
ahh, so the id is just for the DOM. Why is the timestamp being set like this though? The events should have a timestamp in there.
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.
Updated it to try and use the timestamp from the event in 252a933
| Version int `json:"version"` | ||
| Value ldvalue.Value `json:"value"` | ||
| Version int `json:"version"` | ||
| TrackEvents bool `json:"trackEvents"` |
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.
What behavior do we want? This will send an event for every flag eval which is not what will happen in real LD.
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.
Our thought was any flag with an override, we can set trackEvents to be true to mimics like the flag is under an experiment/guarded rollout
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.
Interesting. I guess that kinda makes sense, but the behavior needs to be documented somewhere. Maybe a new events section in the reference guide?
|
|
||
| // Get total count for pagination info | ||
| var totalCount int64 | ||
| countQuery := `SELECT count(DISTINCT(debug_session_key)) FROM debug_events` |
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.
query the other table
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.
it is possible for a debug_session to end up with 0 events, and we did not want to show show those.
The main list query for events also excludes debug_sessions with 0 events
And we purge the orphaned debug_sessions at startup: http://github.com/launchdarkly/ldcli/pull/596/files#diff-bb1801aac503b2e2234ecb22f46b77b6d090c9274966475e2a2873f76166d20dR179
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.
ahhhhh got it. Makes sense
| writer.WriteHeader(http.StatusAccepted) | ||
| } | ||
|
|
||
| func SdkEventsTeeHandler(writer http.ResponseWriter, request *http.Request) { |
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.
ohhhh. I get what this is for now. I think we should move this to not be in the SDK package. Similarly for this observer.
|
|
||
| var observers *model.Observers = model.NewObservers() | ||
|
|
||
| func SdkEventsReceiveHandler(writer http.ResponseWriter, request *http.Request) { |
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.
This is kinda busting the testable MVC-like pattern elsewhere in the dev server. I'd like to see this factored into the model so that we can have a test for the behavior independent of http.
| <div> | ||
| <h3>Events Stream</h3> | ||
| {onToggleStreaming && ( | ||
| <button |
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.
Why aren't launchpad components being used?
|
Picking this back up, doccing the local dev loop:
|
| </Text> | ||
| </Box> | ||
|
|
||
| {debugSessions.length === 0 ? ( |
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.
Getting a crash instead of showing zero-state page here
Uncaught TypeError: can't access property "length", debugSessions is null
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.
Fixed in eb221a6
Resolves an issue where viewing a debug session and then going back in history caused pages to stop re-rendering
618cb61 to
d77388b
Compare
moribellamy
left a comment
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.
LGTM
Add SDK Event to Ldcli:
1.) Adds a new navigation so that uses can switch between multiple pages
2.) Adds new local event debugger:
When a user is viewing the "Events" Page:
A User can View/Delete Debug Sessions:

When A user is viewing a past "Debug Session", they can search for events:
