-
Notifications
You must be signed in to change notification settings - Fork 77
feat: segment telemetry #329
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
…v and prod settings
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 was included as a reviewer before, somehow my comments are mixed as review comments. Hopefully it make sense
@katzino oh, I missed this comment. Yes, it make sense. There are two aspects:
@MQ37 @jirimoravcik what do you think? |
katzino
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.
Pre-approving, just check out that one comment I have for userId
src/telemetry.ts
Outdated
| * @param properties - Event properties for the tool call | ||
| */ | ||
| export function trackToolCall( | ||
| userId: string, |
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 have seen you using crypto.randomUUID() as a fallback but that should be attributes to anonymousId instead. In theory it shouldn't make much of a difference in mixpanel, but Segment is treating userId and anonymousId differently. I would maybe double check with @mr-rajce about it.
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.
Good catch, I was not aware of that. I've changed it to
client.track({
...(userId ? { userId } : { anonymousId: crypto.randomUUID() }),
event: SEGMENT_EVENTS.TOOL_CALL,
properties,
});
… userId is available, use it; otherwise use anonymousId
| // Default store should be available after construction | ||
| const store = server.options.telemetry!.toolCallCountStore!; |
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 does should mean? If we're certain, we can type it in a way that we don't need !. If we're not sure, we should do a check before accessing it.
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've changed this part a bit more. See this comment
In nutshell, there is now private toolCallCountStore: ToolCallCounterStore | undefined; which will be defined only if telemetry is enabled.
But I had to change the tests to
const toolCallCount = server.getToolCallCountStore();
expect(toolCallCount).toBeDefined();
if (!toolCallCount) {
// TypeScript needs this for type narrowing
expect.fail('toolCallCount should be defined');
}
src/types.ts
Outdated
| * Enable or disable telemetry tracking for tool calls. | ||
| * Defaults to true when not set. | ||
| */ | ||
| enabled?: boolean; |
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.
Not sure if this should be optional, the parent object already is.
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.
Yeah, it might be better to be explicit here, so I marked it as required
* Must be explicitly set when telemetry object is provided.
* When telemetry object is omitted entirely, defaults to true (via env var or default).
*/
| /** | ||
| * Tracks a tool call event to Segment. | ||
| * | ||
| * @param userId - Apify user ID (null if not available) |
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.
nullif not available
but the type is just string? Shouldn't it be string | 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.
Yeah, my mistake, I've done some changes and forgot to update it, it is string | null now (to handle anonymousId correctly)
src/telemetry.ts
Outdated
| analyticsClient = new Analytics({ | ||
| writeKey, | ||
| flushAt: 50, | ||
| flushInterval: 5000, |
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.
Can we extract this into a constant that would mention the units? I'd assume it's 5_000 ms, but not sure from this part of code.
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.
Yeah, right, changed to:
flushAt: SEGMENT_FLUSH_AT_EVENTS,
flushInterval: SEGMENT_FLUSH_INTERVAL_MS,
src/mcp/server.ts
Outdated
| private currentLogLevel = 'info'; | ||
| public readonly options: ActorsMcpServerOptions; | ||
| // In-memory storage for tool call counters (used when toolCallCounterStore is not provided) | ||
| private sessionToolCallCounters = new Map<string, number>(); |
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 this can grow infinitely, we should implement some mechanism that would keep a limited number of items in the cache. Maybe a LRUCache would 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.
Good catch. In practice, this shouldn’t happen. When running locally, you usually create a single session, since stdio can’t handle multiple concurrent sessions.
For a remote server, you should provide your own getAndIncrementToolCallCounter with a Redis backend.
BUT if that function isn’t provided, the counter would grow indefinitely
So fixed using LRUCache.
src/mcp/server.ts
Outdated
| * @returns Promise resolving to the new counter value (after increment) | ||
| */ | ||
| private async getAndIncrementToolCallCounter(sessionId: string): Promise<number> { | ||
| return await this.options.telemetry!.toolCallCountStore!.getAndIncrement(sessionId); |
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.
Can we type it so that we get rid of !?
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.
Well, this makes me think I should handle it in a better way (including telemetry envs). Previously, I was modifying input.options, but instead of doing that, I’ve now introduced vars
private telemetryEnabled: boolean | null = null;
private telemetryEnv: TelemetryEnv = DEFAULT_TELEMETRY_ENV;
private toolCallCountStore: ToolCallCounterStore | undefined;
IMO this makes code cleaner, as I don't have to write this.options.telemetry.env etc.
Co-authored-by: Jiří Moravčík <[email protected]>
…ounters` with LRUCache, add telemetry as class instance variables
MQ37
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.
My main question is why the tests/integration/actor.server-sse.test.ts and tests/integration/actor.server-streamable.test.ts were removed? Then only minor comments, otherwise LGTM 👍
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.
note: I don't think we need tests like this. But let's keep that since it is already 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.
I've removed the Actor code in other PR. I did not realize that you were relying on these test to for HTTP and SSE.
src/mcp/server.ts
Outdated
| // Telemetry configuration (resolved from options and env vars in setupTelemetry) | ||
| private telemetryEnabled: boolean | null = null; | ||
| private telemetryEnv: TelemetryEnv = DEFAULT_TELEMETRY_ENV; | ||
| private toolCallCountStore: ToolCallCounterStore | undefined; |
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.
question: Do we actually need this tool call counter logic? Why not just use the timestamp from the analytics that is included automatically and if we are just interested in the total session tool calls count I guess some mixpanel query could easily handle 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.
Just for the record: I check again with Kuba R. and it is not actually required.
Earlier it was proposed by Kuba to track user journey, so I'm going to remove it.
Thanks for pointing it out again!
This PR implements the Segment telemetry.
Implemented based on https://www.notion.so/apify/Apify-MCP-server-analytics-2a1f39950a2280c4a9a5c8154dfc0f1a.
related to https://github.com/apify/ai-team/issues/56 (I would mark completed manually after we verify in prod).
This PR does not implement the
anonymousId(Device ID) yet for unauth requests (coming soon) - I would implement this with the PR where we are going to implement the unauth requests for docs.Other changes:
TELEMETRY_ENABLEDenv var to doppler. Localhost=false, for staging and prod it is enabled.TELEMETRY_ENVwith optionsdevandprodBonus features:
~/.apify/auth.jsonfile that is created by Apify CLI so if you are a CLI user you can use the server without specifying the env var.