Add Inspector support #852
Conversation
| @@ -272,6 +273,8 @@ export class SegmentClient { | |||
|
|
|||
| // flush any stored events | |||
| this.flushPolicyExecuter.manualFlush(); | |||
|
|
|||
| await Inspector.init(); | |||
There was a problem hiding this comment.
Can we make this enable only when running while on debug? You can use this.config.debug to check
| Inspector.trace({ | ||
| timestamp: new Date().toISOString(), | ||
| stage: 'triggered', | ||
| event: event as never, |
| query: { | ||
| platform: Platform.OS, | ||
| sdk: 'React Native', | ||
| appName: await nativeModule.getAppName(), |
There was a problem hiding this comment.
It could receive this as an argument to decouple it from the NativeModules
| "@segment/sovran-react-native": "^1", | ||
| "deepmerge": "^4.2.2", | ||
| "js-base64": "^3.7.2", | ||
| "socket.io-client": "^4.6.2", |
There was a problem hiding this comment.
My concern right now is that socket.io seems to be a huge library. 1MB+ in size.
Metro doesn't do Tree Shaking so it can be a lot of imports that we don't use (and possibly users won't either in prod builds)
Can we refactor this into a standalone package before publishing it?
An idea is to dispatch events at all the places where we currently do a inspector.trace call, the inspector can then subscribe to those events. It could retain the same APIs and TraceData object for simplicity.
If we refactor it into a standalone plugin lib we could even dismiss the debug check I mentioned in a comment below as we could refer people to only install it as devDependency.
| const status = (await this.socket.emitWithAck( | ||
| 'inspector-api', | ||
| sid, | ||
| method, | ||
| args | ||
| )) as number; |
There was a problem hiding this comment.
Do we need to handle any errors in this promise?
| const traces = this.queue; | ||
|
|
||
| const isSuccessfullBootup = | ||
| (await send('start', {})) && (await sendTraces(traces)); |
There was a problem hiding this comment.
I'm curious as to why send the current queue here instead of as the first "prevBatch".
Is it common to experience disconnects in the socket during this step?
| @@ -76,6 +77,15 @@ export class Timeline { | |||
| } | |||
| } | |||
|
|
|||
| Inspector.trace({ | |||
| timestamp: new Date().toISOString(), | |||
| stage: 'delivered', | |||
There was a problem hiding this comment.
Are the stage values an enum? do they show up differently in the inspector depending on the value here?
| stage: 'delivered', | ||
| event: result as never, | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| id: incomingEvent.messageId!, |
There was a problem hiding this comment.
nit: I don't think this should ever ever happen, but maybe use result instead of incomingEvent as technically a plugin can change the messageId
| @@ -103,6 +113,21 @@ export class Timeline { | |||
| } else { | |||
| await pluginResult; | |||
| } | |||
|
|
|||
| if ([PluginType.before, PluginType.enrichment].includes(type)) { | |||
There was a problem hiding this comment.
Should we also notify for after plugins? maybe not as an enriched but as a special stage?
| if ([PluginType.before, PluginType.enrichment].includes(type)) { | ||
| Inspector.trace({ | ||
| timestamp: new Date().toISOString(), | ||
| stage: 'enriched', |
There was a problem hiding this comment.
How about triggering this for all plugin types, switch the stage depending on the type.
As for the name of the plugin it could pull the key for the Destinations (it's the only one that is enforced at the moment):
let stageMap = {
PluginType.enrichment: "enriched",
PluginType.before: "enriched",
PluginType.destination: "delivered",
PluginType.after: "after",
// ...
};
const stage = stageMap[type];
Inspector.trace({
timestamp: new Date().toISOString(),
stage: stage,
event: result as never,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
id: event.messageId!,
enricher: {
name: (type === PluginType.destination) ? plugin.key : undefined,
type: PluginType[type],
},
})
This is take 1 at adding working Inspector support. Feedback/improvements welcome