-
Notifications
You must be signed in to change notification settings - Fork 63
feat: telemetry [WIP] #1932
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?
feat: telemetry [WIP] #1932
Conversation
- Added telemetry support to track document opens for usage-based billing. - Introduced `initTelemetry` method in the Editor class to initialize telemetry based on configuration. - Created tests for telemetry functionality, ensuring correct behavior when enabled/disabled and with various configurations. - Updated Editor and SuperDoc configurations to include telemetry options and license key handling. - Enhanced SuperConverter to manage document creation timestamps for unique identification.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64ce62e12f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #generateIdentifierHash() { | ||
| const docId = this.documentGuid || uuidv4(); | ||
| const created = this.getDocumentCreatedTimestamp() || new Date().toISOString(); | ||
|
|
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.
Keep identifier stable when docId/created missing
When a document lacks a stored docId or a dcterms:created timestamp (common in some legacy or generated DOCX files), #generateIdentifierHash() falls back to uuidv4() and new Date().toISOString(). That means the identifier changes every time the same file is opened, so telemetry will over-count “document opens” and break the “same document across sessions” guarantee. Before this change, the hash was derived from file contents, which was stable across opens. Consider deriving the fallback from the file bytes (or persisting a generated ID into the document) to keep identifiers stable across sessions.
Useful? React with 👍 / 👎.
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.
@andrii-harbour this is really important - we can't overwrite createdTimestamp everytime doc is opened
- Simplified telemetry logging in the Editor class by removing unnecessary comments and enhancing debug output for document information. - Removed redundant isNewFile tracking in SuperDoc, allowing automatic handling based on context.
- Introduced default community license key in the Editor class for telemetry initialization. - Updated `initTelemetry` function to use the default license key when none is provided. - Added tests to verify behavior with default and custom license keys. - Improved documentation for the community license key usage.
caio-pizzol
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.
nice work! just a few comments
| this.documentModified = false; // Track if document has been edited | ||
|
|
||
| // Track if this is a new file created from template | ||
| this.isNewFile = params?.isNewFile || false; |
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.
NewFile means never opened in SuperDoc previously?
| console.debug('[super-editor] Document info:', telemetryPayload); | ||
| this.#telemetry.trackDocumentOpen(documentId || null, documentCreatedAt); | ||
| } catch { | ||
| // Fail silently - telemetry should never break the app |
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.
👍
| #generateIdentifierHash() { | ||
| const docId = this.documentGuid || uuidv4(); | ||
| const created = this.getDocumentCreatedTimestamp() || new Date().toISOString(); | ||
|
|
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.
@andrii-harbour this is really important - we can't overwrite createdTimestamp everytime doc is opened
| * @returns Promise that resolves when data is sent | ||
| */ | ||
| async sendDataToTelemetry(data: TelemetryPayload): Promise<void> { | ||
| console.log('[Telemetry] Sending payload:', payload); |
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.
should be removed - they'll log every payload to the console in production
| setTimeout(() => { | ||
| if (this.isDestroyed) return; | ||
| this.emit('create', { editor: this }); | ||
| this.#trackDocumentOpen(); |
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.
#trackDocumentOpen() is called from both #emitCreateAsync() and mount()
if an editor gets created and then mounted later, this could double-count. might want a guard flag.
| // Generate and cache the hash if not already done | ||
| if (!this.documentHash) { | ||
| this.documentHash = this.#generateIdentifierHash(); | ||
| console.debug('[super-converter] Generated document identifier:', { |
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.
same here
| } else { | ||
| this.resetStatistics(); | ||
| } | ||
| console.log('[Telemetry] Response status:', response.status); |
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 logs should be removed or wrapped in a dev check - especially the payload log which includes all telemetry data (final user would see them when inspecting)
| * Get the dcterms:created timestamp from the already-parsed core.xml | ||
| * @returns {string|null} The created timestamp in ISO format, or null if not found | ||
| */ | ||
| getDocumentCreatedTimestamp() { |
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.
getDocumentCreatedTimestamp() and setDocumentCreatedTimestamp() don't have dedicated tests - might be worth adding since they're key to document identity
| * Set the dcterms:created timestamp in the already-parsed core.xml | ||
| * @param {string} timestamp - The timestamp to set (ISO format) | ||
| */ | ||
| setDocumentCreatedTimestamp(timestamp) { |
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.
the dcterms:created element requires xsi:type="dcterms:W3CDTF" attribute per ECMA-376 Part 2 §11.4 [M4.5] -> verify this
current code only sets text content but doesn't verify the required attribute exists in document and/or create the element if missing
this might cause issues with strict OOXML validators
| superdocVersion: this.superdocVersion, | ||
| browserInfo: this.getBrowserInfo(), | ||
| ...(this.metadata && { metadata: this.metadata }), | ||
| events: [event], |
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.
are we batching those events?
initTelemetrymethod in the Editor class to initialize telemetry based on configuration.