feat: edge sdks should send events to bulk/environment endpoint #256

yusinto · 2023-08-26T06:43:20Z

Edge sdks use clientSideID not sdk-key. We should be able to post events to the eventsUri/bulk/clientSideID endpoint for edge sdks without needing to specify the sdk key in the authorization header.

This is an alternate solution to #217. Both prs are attempting to enable events for edge sdks.

Don't be alarmed with the filecount, it's mostly shell file fixes unrelated to this to include a directive which seems to be needed for local build.

…y prefix.

shortcut-integration · 2023-08-26T06:43:23Z

This pull request has been linked to Shortcut Story #214758: Send events to environment endpoint for edge sdks.

packages/shared/sdk-server-edge/package.json

packages/shared/sdk-server/src/data_sources/defaultHeaders.ts

packages/shared/sdk-server/src/events/EventSender.ts

yusinto · 2023-08-26T07:03:19Z

I still need to test this PR in a real application but based on existing js sdk code, this PR should work.

…work locally.

packages/sdk/cloudflare/example/src/index.ts

packages/shared/sdk-server/src/events/EventSender.ts

scripts/build-doc.sh

packages/sdk/cloudflare/example/src/index.ts

link-dev.sh

yusinto · 2023-08-28T02:37:47Z

I ran the cloudflare example app using this code which uses clientSideID and events seem to be sent.

kinyoklion · 2023-08-28T16:04:34Z

I am not sure about this. The events that can be generated from a client SDK are not 100% the same. So I do not know how the endpoints for a client SDK handle the data.

The server-side SDKs, which these are, generate prerequisite record events. Would the client SDK endpoint for events handle those correctly? If we have a definitive answer, then I can see pursuing it.

kinyoklion · 2023-08-28T16:05:51Z

Secondarily it appears these changes break many contract tests, so some investigation is need there.

kinyoklion

Need to investigate the event test failures in the contract tests.

yusinto · 2023-08-28T18:21:36Z

Would the client SDK endpoint for events handle those correctly

@kinyoklion Thank you for your feedback. I submitted this PR having done due diligence checking the event recorder logic how it handles events for server and client sdks.

My investigation reveals both the /bulk and /events/bulk/{envId} endpoints are handled equally by the postBulkEvents handler. You'll see this postBulkEvents contain the RecordRequest logic you are concerned with so I think this is covered for both server and client sdks.

I understand your concerns though and you want us to be thorough with this kind of hybrid logic because we never had to mix the two before. The edge sdks are unique in this way, it is truly somewhat of a hybrid. This approach opens the door for the future for such sdks so I'm hopeful this works. I will assign someone from the events pipeline to review my work.

I will investigate the contract tests failures, I suspect they are related to the sdk- key prefix which is necessary now. If those tests use some arbitrary strings as sdkKey then those will fail.

yusinto · 2023-08-28T19:10:55Z

Added @samstokes as reviewer from the Events pipeline team.

…point-for-edge

samstokes

LGTM from an event pipeline standpoint. (Leaving it to the SDK team to do the approving.)

Re:

The events that can be generated from a client SDK are not 100% the same. So I do not know how the endpoints for a client SDK handle the data.

I can confirm that both endpoints behave identically in terms of what event payloads they will accept. e.g. even though only browser SDKs generate click events, the /bulk endpoint that handles server-side SDK payloads will happily accept a click event. It all goes into the same Kinesis stream :)

Re the specific concern about prerequisite events:

The server-side SDKs, which these are, generate prerequisite record events. Would the client SDK endpoint for events handle those correctly? If we have a definitive answer, then I can see pursuing it.

I don't think anything in our event pipeline does anything special with prerequisites. Definitely the events endpoint handlers don't.

(AFAIK there's nothing special about prerequisite events, right - they're just regular evaluation events? And the only special consideration is that client-side SDKs don't send them.

packages/shared/sdk-server-edge/src/api/LDClient.ts

packages/shared/sdk-server/src/events/EventSender.ts

yusinto · 2023-08-28T21:23:22Z

from a security standpoint it'd be nice if there was a clearer distinction between sdkKey and clientSideID in the code

Thanks @samstokes ! I have improved the code to provide a better distinction between sdkKey and clientSideID.

packages/sdk/cloudflare/example/src/index.ts

yusinto · 2023-09-05T18:15:09Z

@ldhenry pls re-review when able. Thank you

ldhenry · 2023-09-08T13:56:16Z

@ldhenry pls re-review when able. Thank you

Hey @yusinto, I was hoping to get some sort of evidence that this is working from our alpha customers before making this more widely available. However, I can't see any usage in honeycomb. Do you know if there is a better way to see adoption?

yusinto · 2023-09-13T22:00:06Z

Do you know if there is a better way to see adoption?

Yep we can try querying cmetrics directly following these instructions.

samstokes

Nothing to add, just commenting so GitHub will stop reminding me about this PR. If there's anything that does need my feedback please feel free to "re-request review".

…point-for-edge

yusinto · 2023-11-08T20:33:11Z

@ldhenry @kinyoklion I resolved conflicts and made minor updates and added unit tests. Please re-review. Thank you.

Tested with the example app and verified in the UI.

mmrj · 2023-11-09T20:30:03Z

packages/sdk/cloudflare/example/src/index.ts

To the Vercel and/or Akamai examples also need to be updated? I initially thought this change only applied to Cloudflare because only the Cloudflare example was changed.

Those examples can be updated, but not necessary. I'll leave that to @ldhenry .

yusinto · 2023-11-12T03:03:07Z

@ldhenry please approve so this can be merged. Thank you.

🤖 I have created a release *beep* *boop* --- <details><summary>@launchdarkly/akamai-edgeworker-sdk-common: 1.0.3</summary> ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from ^2.0.2 to ^2.1.0 </details> <details><summary>@launchdarkly/akamai-server-base-sdk: 2.0.3</summary> ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.0.2 to ^1.0.3 * @launchdarkly/js-server-sdk-common bumped from ^2.0.2 to ^2.1.0 </details> <details><summary>@launchdarkly/akamai-server-edgekv-sdk: 1.0.11</summary> ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.0.2 to ^1.0.3 * @launchdarkly/js-server-sdk-common bumped from ^2.0.2 to ^2.1.0 </details> <details><summary>cloudflare-server-sdk: 2.3.0</summary> ## [2.3.0](cloudflare-server-sdk-v2.2.3...cloudflare-server-sdk-v2.3.0) (2023-11-14) ### Features * edge sdks should send events to bulk/environment endpoint ([#256](#256)) ([f45910f](f45910f)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/js-server-sdk-common-edge bumped from 2.0.2 to 2.1.0 </details> <details><summary>js-sdk-common: 2.1.0</summary> ## [2.1.0](js-sdk-common-v2.0.0...js-sdk-common-v2.1.0) (2023-11-14) ### Features * edge sdks should send events to bulk/environment endpoint ([#256](#256)) ([f45910f](f45910f)) </details> <details><summary>js-server-sdk-common: 2.1.0</summary> ## [2.1.0](js-server-sdk-common-v2.0.2...js-server-sdk-common-v2.1.0) (2023-11-14) ### Features * edge sdks should send events to bulk/environment endpoint ([#256](#256)) ([f45910f](f45910f)) ### Bug Fixes * Better handle waiting for initialization for failure cases. ([#314](#314)) ([16515df](16515df)), closes [#312](#312) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-sdk-common bumped from 2.0.0 to 2.1.0 </details> <details><summary>js-server-sdk-common-edge: 2.1.0</summary> ## [2.1.0](js-server-sdk-common-edge-v2.0.2...js-server-sdk-common-edge-v2.1.0) (2023-11-14) ### Features * edge sdks should send events to bulk/environment endpoint ([#256](#256)) ([f45910f](f45910f)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from 2.0.2 to 2.1.0 </details> <details><summary>@launchdarkly/node-server-sdk: 9.0.3</summary> ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from 2.0.2 to 2.1.0 </details> <details><summary>@launchdarkly/node-server-sdk-dynamodb: 6.0.3</summary> ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/node-server-sdk bumped from 9.0.2 to 9.0.3 </details> <details><summary>@launchdarkly/node-server-sdk-redis: 4.0.3</summary> ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/node-server-sdk bumped from 9.0.2 to 9.0.3 </details> <details><summary>vercel-server-sdk: 1.2.0</summary> ## [1.2.0](vercel-server-sdk-v1.1.7...vercel-server-sdk-v1.2.0) (2023-11-14) ### Features * Support analytics events in the vercel SDK. ([#316](#316)) ([cc41db4](cc41db4)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common-edge bumped from 2.0.2 to 2.1.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>

LaunchDarklyReleaseBot added 2 commits August 25, 2023 23:36

feat: implement useEnvironmentEndpoint logic by inspecting the sdk-ke…

a2dbfe4

…y prefix.

fix: broken tests.

cada2f6

yusinto requested review from kinyoklion and louis-launchdarkly as code owners August 26, 2023 06:43

yusinto marked this pull request as draft August 26, 2023 06:43

yusinto requested a review from ldhenry August 26, 2023 06:43

fix: wrong events path for clientSideID

37bdbf4

yusinto commented Aug 26, 2023

View reviewed changes

packages/shared/sdk-server-edge/package.json Outdated Show resolved Hide resolved

packages/shared/sdk-server/src/data_sources/defaultHeaders.ts Outdated Show resolved Hide resolved

packages/shared/sdk-server/src/events/EventSender.ts Outdated Show resolved Hide resolved

fix: added shell directive. added link-dev for cloudflare/example to …

cfbc545

…work locally.

yusinto commented Aug 28, 2023

View reviewed changes

chore: remove debug statements.

2874194

yusinto marked this pull request as ready for review August 28, 2023 02:54

kinyoklion requested changes Aug 28, 2023

View reviewed changes

yusinto requested a review from samstokes August 28, 2023 19:09

Merge branch 'main' into yus/sc-214758/send-events-to-environment-end…

8bcd58a

…point-for-edge

samstokes reviewed Aug 28, 2023

View reviewed changes

packages/shared/sdk-server-edge/src/api/LDClient.ts Outdated Show resolved Hide resolved

packages/shared/sdk-server/src/events/EventSender.ts Outdated Show resolved Hide resolved

yusinto added 3 commits August 28, 2023 13:59

chore: better support for event sender analytics paths (#259)

1908227

chore: rollback unit test sdk- prefix changes.

11cb96a

chor: add comments

a3d3132

yusinto requested review from kinyoklion and samstokes August 28, 2023 21:15

Update packages/shared/sdk-server-edge/src/api/LDClient.ts

429e92a

ldhenry mentioned this pull request Aug 30, 2023

DO NOT MERGE: feat: add the ability to send events using the Vercel Edge SDK #217

Closed

ldhenry requested changes Aug 30, 2023

View reviewed changes

packages/sdk/cloudflare/example/src/index.ts Outdated Show resolved Hide resolved

packages/sdk/cloudflare/example/src/index.ts Outdated Show resolved Hide resolved

chore: flush events in waitUntil. improved handler types.

ad7d012

yusinto requested a review from ldhenry August 31, 2023 00:49

samstokes reviewed Sep 15, 2023

View reviewed changes

yusinto added 9 commits September 21, 2023 15:10

Merge branch 'main' into yus/sc-214758/send-events-to-environment-end…

303e3ae

…point-for-edge

fix: remove link-dev and use yarn workspaces.

44c8044

fix: unit tests due to missing ctx. update jest version.

2abeb13

chore: fix flush callback arg types.

8f1ad45

fix: close client to fix jest open handle warnings

8b5c0d9

Merge branch 'main' into yus/sc-214758/send-events-to-environment-end…

6cb2165

…point-for-edge

chore: resolve conflicts.

aae16d8

chore: in cloudflare example, rename sdkKey to clientSideID.

2acad70

chore: added unit tests for edge sdk.

5da020e

yusinto requested a review from kinyoklion November 8, 2023 20:29

yusinto added 2 commits November 8, 2023 12:55

chore: update cloudflare sdk dep version

474cdf7

chore: set test data trackEvents to true

55bf9af

louis-launchdarkly approved these changes Nov 9, 2023

View reviewed changes

mmrj reviewed Nov 9, 2023

View reviewed changes

yusinto enabled auto-merge (squash) November 12, 2023 03:02

ldhenry approved these changes Nov 12, 2023

View reviewed changes

yusinto merged commit f45910f into main Nov 12, 2023
15 checks passed

yusinto deleted the yus/sc-214758/send-events-to-environment-endpoint-for-edge branch November 12, 2023 22:56

github-actions bot mentioned this pull request Nov 12, 2023

chore: release main #313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: edge sdks should send events to bulk/environment endpoint #256

feat: edge sdks should send events to bulk/environment endpoint #256

yusinto commented Aug 26, 2023 •

edited

Loading

shortcut-integration bot commented Aug 26, 2023

yusinto commented Aug 26, 2023

yusinto commented Aug 28, 2023

kinyoklion commented Aug 28, 2023

kinyoklion commented Aug 28, 2023

kinyoklion left a comment

yusinto commented Aug 28, 2023 •

edited

Loading

yusinto commented Aug 28, 2023

samstokes left a comment

yusinto commented Aug 28, 2023

yusinto commented Sep 5, 2023

ldhenry commented Sep 8, 2023

yusinto commented Sep 13, 2023

samstokes left a comment

yusinto commented Nov 8, 2023 •

edited

Loading

mmrj Nov 9, 2023

yusinto Nov 12, 2023

yusinto commented Nov 12, 2023

feat: edge sdks should send events to bulk/environment endpoint #256

feat: edge sdks should send events to bulk/environment endpoint #256

Conversation

yusinto commented Aug 26, 2023 • edited Loading

shortcut-integration bot commented Aug 26, 2023

yusinto commented Aug 26, 2023

yusinto commented Aug 28, 2023

kinyoklion commented Aug 28, 2023

kinyoklion commented Aug 28, 2023

kinyoklion left a comment

Choose a reason for hiding this comment

yusinto commented Aug 28, 2023 • edited Loading

yusinto commented Aug 28, 2023

samstokes left a comment

Choose a reason for hiding this comment

yusinto commented Aug 28, 2023

yusinto commented Sep 5, 2023

ldhenry commented Sep 8, 2023

yusinto commented Sep 13, 2023

samstokes left a comment

Choose a reason for hiding this comment

yusinto commented Nov 8, 2023 • edited Loading

mmrj Nov 9, 2023

Choose a reason for hiding this comment

yusinto Nov 12, 2023

Choose a reason for hiding this comment

yusinto commented Nov 12, 2023

yusinto commented Aug 26, 2023 •

edited

Loading

yusinto commented Aug 28, 2023 •

edited

Loading

yusinto commented Nov 8, 2023 •

edited

Loading