-
Notifications
You must be signed in to change notification settings - Fork 0
Telemetry implementation #4
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
base: main
Are you sure you want to change the base?
Conversation
This reverts commit f06e652.
Looks good to me one! One thought: Say I do something like https://chatgpt.com/share/6899f0b4-3a08-800a-a8ef-7422887e40e6 Would be sorta cool if that's not a hallucination. |
Agree it would be nice to have the types even if there is not code verifying what is passed at runtime. I think Eric/ChatGPT is right that you can do overloads. Something like this? type TelemetryKind = "console" | "network" | "etc...";
interface ConsoleData {
message: string
}
interface NetworkData {
statusCode: number
}
function addTelemetry(kind: "console", data: ConsoleData) : void;
function addTelemetry(kind: "network", data: NetworkData) : void;
function addTelemetry(kind: TelemetryKind, data: ConsoleData | NetworkData) : void {
// do things...
}
// Usage
addTelemetry("console", { message: "test" }); // works
addTelemetry("network", { message: "test" }); // compile error
addTelemetry("network", { statusCode: 500 }); // works |
Another way we could do this is with actual types, which could be enforced at runtime. TrackJS.addTelemetry(new ConsoleTelemetry(timestamp(), "log", "message")); Then, rather than using the type string at all, we just use EDIT: Another benefit of this is that it gives us a good place to do validation, like saying URLs can't be 1000char+, or log messages have to be less than X" |
That might be the most straightforward way to do it. |
Yep, I considered suggesting that too but figured you were keeping the type of telemetry separate from the payload maybe for serialization purposes (like that way the type name is not sent as part of the payload, just to keep it smaller and such) I don't have a strong preference one way or another - I don't mind the way you've got it now, so with some type constraint/maps might be just fine? Otherwise if you're feeling like the "more" OOP approach approach is better that's fine with me too! |
I went with the @BrandesEric / @J-Griffin Typescript approach. I didn't like the idea of revealing internal types like But since the syntax really only prevented Typescript from doing something wrong, I wrote another e2e test that was just plain-old JavaScript and tried to do things incorrectly to make sure something threw an error rather than silently sending crap to our API. It's not bullet proof, but I didn't really want to build a full type-check-enforcement thingy. |
This is a basic/manual implementation of TrackJS telemetry.
Can accept console, network, navigation, or visitor events. There is no normalization or serialization on what you send (yet), since it's directly on our API, it trusts that you are not doing something silly. But maybe it should do some limiting/validation on the fields?
I also tested that an object can be updated after being added to the log. This is useful for network where it will be completed at a later time with more information.
It does not clear the Telemetry log after sending an error. I went to implement this, but noticed that the Node agent doesn't do this intentionally because we thought at the time that including all the telemetry history with the 2,3,...nth error is useful. I like this approach I think, but I wanted to call it out and run it by yall.