Add ability to pass custom http client into node by silesky · Pull Request #880 · segmentio/analytics-next

silesky · 2023-07-05T20:31:08Z

Package: @segment/analytics-node.

Please see the types/default implementation here: packages/node/src/lib/http-client.ts

This is based on work / research from @MichaelGHSeg!

Use a custom fetch-like implementation with proxy (simple, recommended)

import { HTTPFetchFn } from '@segment/analytics-node'
import axios from 'axios'

const httpClient: HTTPFetchFn = async (url, options) => {
  return axios({
    url,
    proxy: {
        protocol: 'http',
        host: 'proxy.example.com',
        port: 8886,
        auth: {
          username: 'user',
          password: 'pass',
        },
      },
    ...options,
  })
}

const analytics = new Analytics({
  writeKey: '<YOUR_WRITE_KEY',
  httpClient,
})

Augment the default HTTP Client

import { FetchHTTPClient, HTTPClientRequest  } from '@segment/analytics-node' 
 
class MyClient extends FetchHTTPClient {
  async makeRequest(options: HTTPClientRequest) {
    return super.makeRequest({
         ...options, 
         headers: { ...options.headers, foo: 'bar'  }
      }})
  }
}

const analytics = new Analytics({ 
  writeKey: '<YOUR_WRITE_KEY>', 
  httpClient: new MyClient() 
})

Option 2: Completely override the full HTTPClient (Advanced, you probably don't need to do this)

import { HTTPClient, HTTPClientRequest } from '@segment/analytics-node'

class CustomClient implements HTTPClient {
  async makeRequest(options: HTTPClientRequest) {
    return someRequestLibrary(options.url, { 
      method: options.method,
      body: JSON.stringify(options.data) // serialize data
      headers: options.headers,
    })
  }
}
const analytics = new Analytics({ 
  writeKey: '<YOUR_WRITE_KEY>', 
  httpClient: new CustomClient() 
})

changeset-bot · 2023-07-05T20:31:11Z

🦋 Changeset detected

Latest commit: 84ac43b

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

This PR includes changesets to release 1 package

Name	Type
@segment/analytics-node	Minor

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

wip wip wip wip wip wip wip wip wip wip wip

danieljackins · 2023-07-06T23:53:17Z

Was this added accidentally?

Good catch!

MichaelGHSeg · 2023-07-07T03:21:56Z

-      )
      try {
-        const requestInit = {
-          signal: signal,


Did you have a specific fetch replacement library in mind that doesn't support 'signal' here? I have questions about simultaneously allowing access to tinker with our internals and passing around non-standard objects.

All fetch libraries support AbortSignal, but not all HTTP Libraries do (e.g Request).

That's why signal is present in the FetchHTTPClient class/interface but not the HTTPClient interface -- it's fetch specific.

Does this answer your concern? I am not entirely sure I am following you on you standard objects / monkeying with internals -- maybe you can elaborate on what part of the examples (see PR description) you feel is problematic. I wanted to allow a couple layers extensibility -- users can swap out the default fetch client, or, if they need, replace the entire http client with something that doesn't use fetch / have any relationship with the fetch interface at all.

I just wanted to know what the impetus was. Figured you had a reason for moving some of the fiddly bits into the user replaceable thing that they then have to repeat.

chrisradek · 2023-07-10T20:35:22Z

      method: string
      headers: Record<string, string>
-      body: string
+      body: Record<string, any>


What's the reasoning for changing this from a string to an object? Seems like it saves us from having to do a JSON.parse() in our tests, but I'd also expect an HTTP body to be a string if I'm specifically listening for HTTP requests/want to know exactly what got sent to the service.

(Also this would technically be a breaking change going from a string to an object - someone already doing a JSON.parse() would now encounter an error, though I don't expect many users of this API yet)

The serialization part is the client's responsibility, and I didn't want the http client to have take an emitter -- so the emitting part now happens in the publisher code (before the request is passed to the http client). I saw no downsides to this change, and only upsides.

chrisradek · 2023-07-10T21:55:55Z

+   * Timeout in milliseconds
+   * @example 10000
+   */
+  timeout: number


I've seen a lot of different HTTP-related timeouts (timeout to establish connection, socket idle timeout, time for response headers, time for full response body) - versus browsers that were way more limited with what XHR provided. Can we be more detailed on what this timeout covers?

Good point. We currently support httpRequestTimeout which is "timeout for full response body" -- I can rename it to match our config.

chrisradek · 2023-07-10T22:03:32Z

+   * JSON data to be sent with the request (will be stringified)
+   * @example { "batch": [ ... ]}
+   */
+  data: Record<string, any>


It feels a little weird for an HTTP client to have data instead of body. (headers and query params are data too!)

How come we accept an object rather than a string/byte array here? I would expect the HTTP Client to only be concerned with making the HTTP request, not transforming the body.

I can see the possibility of wanting to change the data before sending to the server and hence wanting to have the object and not parse from JSON.

But we have already a few places to do these operations, so I agree with this, let's keep the HTTP Client dumb to discourage having too much logic here

The reason I used data here is:

to distinguish the higher level representation of the body (JSON object) from the serialized body (string) -- fetch takes a string

To allows people to use a custom JSON serializer if they want.

To allow people to make modifications to "data/payload" without deserializing and re-serializing (expensive)

This is what axios uses, so it's a comment convention

The HTTPClient is meant to be generic in terms of what we support (for example, if we didn't support timeout, that wouldn't be an option).

PS: this pattern is also used by Stripe in their SDK's HTTPClient.

PPS:The shape will be { batch: [{ ..... }] }

chrisradek · 2023-07-10T22:07:04Z

+ * @link https://developer.mozilla.org/en-US/docs/Web/API/Response
+ */
+export interface HTTPResponse {
+  ok: boolean


Kind of wonder if we really need ok - it's just a boolean that is true if status if 200-299. We get status already so could check that ourselves (then if someone uses an HTTP client that doesn't return ok, they don't have to worry about it)

chrisradek · 2023-07-10T22:08:12Z

+    }
+
+    return this._fetch(options.url, requestInit).finally(() =>
+      clearTimeout(timeoutId)


Ah I see, timeout is time to response headers (to answer my earlier question 😄)

oscb

Looks good. I +1 on some of the comments before, I think the most important one would be to remove the ok from the responses to prevent weird issues where it's missed to turn on (or the other way around.

oscb · 2023-07-11T16:33:01Z

+   * JSON data to be sent with the request (will be stringified)
+   * @example { "batch": [ ... ]}
+   */
+  data: Record<string, any>


I can see the possibility of wanting to change the data before sending to the server and hence wanting to have the object and not parse from JSON.

But we have already a few places to do these operations, so I agree with this, let's keep the HTTP Client dumb to discourage having too much logic here

oscb

👍

silesky requested review from MichaelGHSeg and chrisradek July 5, 2023 20:31

silesky mentioned this pull request Jul 5, 2023

Pluggable custom http client wrapper #877

Closed

silesky force-pushed the add-ability-to-pass-custom-node-client branch 3 times, most recently from 47c2e34 to e9dca8d Compare July 5, 2023 20:51

silesky changed the title ~~Add ability to pass custom node client~~ Add ability to pass custom http client into node Jul 5, 2023

MichaelGHSeg approved these changes Jul 6, 2023

View reviewed changes

silesky requested review from danieljackins, oscb and zikaari July 6, 2023 03:57

MichaelGHSeg mentioned this pull request Jul 6, 2023

Adding info about new http client segmentio/segment-docs#4989

Merged

silesky added 4 commits July 6, 2023 15:17

Seth - example refactor

70c124a

wip wip wip wip wip wip wip wip wip wip wip

wip

4143e98

wip

79c6833

wip

a6a0d95

silesky force-pushed the add-ability-to-pass-custom-node-client branch from e9dca8d to a6a0d95 Compare July 6, 2023 23:14

wip

5509a84

danieljackins approved these changes Jul 7, 2023

View reviewed changes

wip

22cb6e5

MichaelGHSeg reviewed Jul 7, 2023

View reviewed changes

silesky added 8 commits July 7, 2023 01:51

Update emitter.ts

02fcf9c

Update emitter.ts

7c59f8f

wip

cac9d81

wip

3d8d734

wip

230bc9e

wip

0af7e85

wip

4e3dbb1

wip

e1da819

silesky added 3 commits July 7, 2023 15:54

wip

e811273

wip

6d628ba

wip

e6952cc

chrisradek reviewed Jul 10, 2023

View reviewed changes

oscb suggested changes Jul 11, 2023

View reviewed changes

silesky added 7 commits July 12, 2023 13:50

update tests/types and respond to feedback

7ba6558

add test

bc4786e

change timeout to 'httpRequestTimeout'

5fdfbad

update docstring

5941180

wip

977d189

wip

d3d694f

wip

84ac43b

silesky force-pushed the add-ability-to-pass-custom-node-client branch from 9c43c89 to 84ac43b Compare July 12, 2023 20:20

oscb approved these changes Jul 13, 2023

View reviewed changes

silesky merged commit 5f50363 into master Jul 13, 2023

silesky deleted the add-ability-to-pass-custom-node-client branch July 13, 2023 16:23

github-actions Bot mentioned this pull request Jul 13, 2023

Version Packages #887

Merged

Conversation

silesky commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use a custom fetch-like implementation with proxy (simple, recommended)

Augment the default HTTP Client

Option 2: Completely override the full HTTPClient (Advanced, you probably don't need to do this)

Uh oh!

changeset-bot Bot commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

silesky commented Jul 5, 2023 •

edited

Loading

changeset-bot Bot commented Jul 5, 2023 •

edited

Loading

silesky Jul 7, 2023 •

edited

Loading

silesky Jul 11, 2023 •

edited

Loading