Skip to content
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

feat(shared): SDK telemetry #2154

Merged
merged 37 commits into from
Nov 22, 2023
Merged

feat(shared): SDK telemetry #2154

merged 37 commits into from
Nov 22, 2023

Conversation

BRKalow
Copy link
Member

@BRKalow BRKalow commented Nov 17, 2023

Description

Introduces opt-out telemetry collection from our SDKs by way of a new TelemetryCollector class. Currently, this supports collection both on the client, by way of our core Clerk instance, and on the server, by way of the pre-initialized @clerk/backend client.

Client collection:

Clerk.telemetry.record({ event: 'EVENT', payload: { ... } })

Server collection:

clerkClient.telemetry.record({ event: 'EVENT', payload: { ... } })

The clerkClient factory from @clerk/backend now accepts sdkMetadata, which is passed from each SDK. This metadata is then passed to TelemetryCollector and included in telemetry events. Currently, collection is only done from development instances.

Opting-out

To opt-out, we respect a CLERK_TELEMETRY_DISABLE environment variable. Frameworks with unique environment variable prefixes, such as Next's NEXT_PUBLIC_ will also disable telemetry collection.

Docs

Documentation is being prepared here: clerk/clerk-docs#496

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

@BRKalow BRKalow requested a review from a team November 17, 2023 05:14
Copy link

changeset-bot bot commented Nov 17, 2023

🦋 Changeset detected

Latest commit: a783aba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
gatsby-plugin-clerk Patch
@clerk/clerk-js Patch
@clerk/clerk-sdk-node Patch
@clerk/backend Patch
@clerk/fastify Patch
@clerk/nextjs Patch
@clerk/shared Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/types Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -0,0 +1,155 @@
import 'cross-fetch/polyfill';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch isn't available in the jsdom jest environment 😢, so adding the polyfill here.

Comment on lines 186 to 218
#scheduleFlush(): void {
// On the server, we want to flush immediately as we have less guarantees about the lifecycle of the process
if (typeof window === 'undefined') {
this.#flush();
return;
}

const isBufferFull = this.#buffer.length >= this.#config.maxBufferSize;
if (isBufferFull) {
// If the buffer is full, flush immediately to make sure we minimize the chance of event loss.
// Cancel any pending flushes as we're going to flush immediately
if (this.#pendingFlush) {
const cancel = typeof cancelIdleCallback !== 'undefined' ? cancelIdleCallback : clearTimeout;
cancel(this.#pendingFlush);
}
this.#flush();
return;
}

// If we have a pending flush, do nothing
if (this.#pendingFlush) return;

if ('requestIdleCallback' in window) {
this.#pendingFlush = requestIdleCallback(() => {
this.#flush();
});
} else {
// This is not an ideal solution, but it at least waits until the next tick
this.#pendingFlush = setTimeout(() => {
this.#flush();
}, 0);
}
}
Copy link
Member Author

@BRKalow BRKalow Nov 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my attempt to implement buffering in a way that mitigates potential data loss. It uses requestIdleCallback to schedule sending events during idle time. When an event is recorded, it schedules a flush during idle, and if the buffer limit is reached it immediately flushes the events.

The scheduling of a flush on every record might be a tad aggressive, but we can tweak later if we want.

Comment on lines 231 to 235
.catch(() => void 0)
.then(() => {
this.#buffer = [];
})
.catch(() => void 0);
Copy link
Member Author

@BRKalow BRKalow Nov 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catch into the void to avoid leaking any errors here. ESLint complained that the chain had to end in a .catch, hence the final .catch as well.

Copy link
Contributor

@jescalan jescalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small thoughts:

  1. Should we add a post-install message notifying about telemetry and how to opt out as is done in nextjs etc here?
  2. Can we get events sent for hook usage here too, or is that planned for a later PR?


fetchSpy.mockRestore();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests 😍

@BRKalow
Copy link
Member Author

BRKalow commented Nov 20, 2023

@jescalan

Should we add a post-install message notifying about telemetry and how to opt out as is done in nextjs etc here?

Yes, good reminder! I'll add this.

Can we get events sent for hook usage here too, or is that planned for a later PR?

Will open a follow-up PR with hooks usage events 👍

packages/types/src/clerk.ts Show resolved Hide resolved
packages/shared/src/telemetry/collector.ts Outdated Show resolved Hide resolved
packages/shared/src/telemetry/collector.ts Show resolved Hide resolved
packages/shared/src/telemetry/collector.ts Show resolved Hide resolved
packages/shared/src/telemetry/collector.ts Show resolved Hide resolved
packages/shared/src/telemetry/collector.ts Outdated Show resolved Hide resolved
packages/shared/package.json Show resolved Hide resolved
Copy link
Contributor

@jescalan jescalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change suggestion, feel free to reject

@BRKalow BRKalow added this pull request to the merge queue Nov 22, 2023
Merged via the queue into main with commit 40ac4b6 Nov 22, 2023
7 checks passed
@BRKalow BRKalow deleted the brk.feat/telemetry-spike branch November 22, 2023 15:49
desiprisg pushed a commit that referenced this pull request Nov 23, 2023
* feat(clerk-js): Spike out telemetry collection

* feat(clerk-js): Add sampling rate and verbose option

* feat(shared): Move TelemtryCollector into shared, make it isomorphic

* feat(nextjs): Add telemtry collection for authMiddleware

* feat(shared): Support passing multiple events to the telemetry endpoint

* feat(shared): Abstract fetch into sendEvent

* feat(shared): Refactor SDK metadata handling to get accurate value

* feat(shared): Update endpoint, don't use no-cors

* fix(nextjs): Fix merge conflict

* feat(shared): Add batching to the telemetry collector

* feat(shared): Add opt-out support for SDK telemetry (#2099)

* feat(clerk-js): Add TelemetryCollector instance to backend client (#2134)

* chore(repo): Add changeset

* fix(shared): Fix tests that rely on fetch

* feat(shared): Set production endpoint

* feat(clerk-js): Make telemtry public, expose via isomorphicClerk

* feat(shared): Add doc on the TelemetryCollector

* feat(clerk-js): Support telemetry: false as a way to disable telemetry

* chore(shared): Tweak comment

* chore(shared): Tweak comment

* fix(clerk-sdk-node): Add PACKAGE_ constants for tests

* fix(fastify): Define PACKAGE_ constants in test environment

* feat(shared): Refactor telemetry instrumentation to rely on helpers

* feat(shared): Add telemetry notice

* feat(shared): Add postinstall script

* chore(shared): Add word

* chore(shared): Swallow errors in postinstall

* chore(repo): Allow scripts directory in subpaths-workaround

* feat(shared): Adjust types to enforce the event structure from event factory methods

* chore(shared): Simplify telemetry.record() API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants