node-datadog-metrics icon indicating copy to clipboard operation
node-datadog-metrics copied to clipboard

Consider dropping `@datadog/datadog-api-client`?

Open Mr0grog opened this issue 1 year ago • 4 comments

One thing I’ve been thinking about is dropping our dependency on the official @datadog/datadog-api-client package. Folks value and use this package over that official one because of simplicity, and it is a pretty heavy-weight dependency for us to bring in. We don’t actually use much of its functionality or features, and it mostly serves as an HTTP client for us.

Would it be worth dropping this dependency and using the HTTP API directly? I currently plan to make a major, breaking release in January 2025 (see #131), and that might be a good time to upgrade the minimum Node.js version to 18, where fetch is built-in and we wouldn’t need to replace the Datadog client with anything at all.

This might not be worthwhile! It adds (a little) maintenance burden (mainly auth and error handling), and keeping a minimal dependency tree is probably not critical to most of our users here (if it is to you, please comment!).

Mr0grog avatar Nov 12 '24 22:11 Mr0grog

Related note: I think one reason datadog-ci may still on v0.9.x of this package is because it needs proxy support, and the right way to do that changed in v0.10.0 (when we started using @datadog/datadog-api-client). See their implementation and this issue about proxies.

If we make this change, it would be good to ensure there’s a nice way to set up proxying (in the worst case, someone can still provide a custom reporter, but it would be nice if they didn’t have to).

Mr0grog avatar Nov 28 '24 00:11 Mr0grog

I think a first implementation of this should replace @datadog/datadog-api-client with cross-fetch, which allows us to stay compatible with Node.js 12.

In a second release, we can update to Node.js 18+ and use the built-in fetch.

Mr0grog avatar Dec 06 '24 00:12 Mr0grog

Some additional notes here:

  • Our biggest user is datadog-ci, which is currently marked as compatible with Node.js v14, so we may want to consider not moving to v18. Either way, it may still be worth moving to direct HTTP requests instead of using @datadog/datadog-api-client.

  • I think the proxy issue brought up in datadog-ci is pretty valuable to support, and it turns out that fetch + proxy is complicated:

    • The standard WHATWG Fetch does not allow you to control proxying at all, but a lot of runtimes add features for it and userland libraries also support it.
    • cross-fetch is a little complicated because it is a placeholder for whatever fetch is available…
      • In browsers, you get the built-in fetch, which doesn’t have a way to do it. BUT there’s not really a way to do it in browsers anyway, so that’s OK (also we aren’t worried about first-class browser support here, since cases where you want to expose your Datadog API key to a browser are rare).
      • In React-Native, you get the built-in fetch, and I’m not clear what’s possible here. Needs more reasearch, although we also aren’t worried about first-class support here for similar reasons as browsers. (Some issues I tripped over hint that React-Native’s fetch supports an agent option, which works like the old agent option we used to support when we were based on dogApi.)
      • In other environments, you get Node-fetch, which takes an agent option like we used to support. Pretty straightforward.
      • Need to do more checking to find out what happens if we supply agent to an underlying implementation that does not support it, or if we can detect what underlying implementation we wound up with and supply agent if supported.
    • node-fetch takes an agent option we can supply or pass through. See above.
    • axios takes an httpsAgent option like node-fetch’s agent, or takes a proxy object with various options.
    • undici takes a dispatcher option that can be an instance of ProxyAgent (which it provides).
    • got takes an agent option (more-or-less like node-fetch).
    • Node.js’s built-in fetch is based on Undici but does not support the dispatcher option on a single request (just globally, and we should not be doing anything global that could affect the user’s other HTTP requests). Discussion about built-in support for Node.js is still ongoing.
    • Bun’s built-in fetch takes a proxy option that is a string of the same format you’d normally provide the HTTP_PROXY or HTTPS_PROXY environment variable for lots of CLI tools.
    • Deno’s fetch takes a client option that you can set up with a proxy URL.

Ideally, we’d be able to use the fetch built-ins for all environments and just supply proxy info in the relevant format for that environment (still a bit of a pain), but Node.js just doesn’t support it with fetch at all, and also doesn’t have built-in proxy support of any kind (you can get an agent via the proxy-agent NPM module).

Another option is to choose a userland module (which we kind of need anyway if we are going to stay compatible with Node.js v14), most of which have a way to do proxies.

  • Node-fetch doesn’t have built-in support but does take an agent. But…
    • I’d prefer not to also include proxy-agent for this not-super-common case and
    • I think exposing an under-the-hood thing like agent in our API is an anti-pattern; it prevents us from making low-level changes that would otherwise be non-breaking for users. See also the whole cause of this mess, when we made Dogapi’s agent option available.
  • Got has the same issues.
  • Axios is huge! (Still much smaller than @datadog/datadog-api-client, though.)
  • Undici is also pretty big (but only half of Axios) and requires us to move back several major versions to an unsupported one to get Node.js v12 support. The oldest supported line is v5, which supports Node.js 18+.

Honestly the straightforward option maybe be to either not worry about this option (someone can supply their own reporter) or make it possible to provide a custom fetch implementation to the built-in DatadogReporter. (Yes this is kind of like supplying an agent, but at least fetch is basically a language-level standard now, unlike agents, which are particular to Node.js.) For example:

import metrics from 'datadog-metrics';
import { HttpsProxyAgent } from 'https-proxy-agent';
import fetch as nodeFetch from 'node-fetch';

const agent = new HttpsProxyAgent('http://my-proxy-url.com:1234');
metrics.init({
  fetch (url, options) {
    return nodeFetch(url, { ...options, agent });
  }
});

Or:

import metrics from 'datadog-metrics';
import { HttpsProxyAgent } from 'https-proxy-agent';
import fetch as nodeFetch from 'node-fetch';

const agent = new HttpsProxyAgent('http://my-proxy-url.com:1234');
const reporter = new metrics.DatadogReporter({
  fetch (url, options) {
    return nodeFetch(url, { ...options, agent });
  }
});
metrics.init({ reporter });

Or:

import metrics from 'datadog-metrics';
import { HttpsProxyAgent } from 'https-proxy-agent';
import fetch as nodeFetch from 'node-fetch';

class ProxyReporter extends metrics.DatadogReporter {
  constructor (options) {
    super(options);
    this.agent = new HttpsProxyAgent('http://my-proxy-url.com:1234');
  }

  fetch (url, options) {
    return nodeFetch(url, { ...options, agent: this.agent });
  }
}

metrics.init({ reporter: new ProxyReporter() });

The last one is a bit of a bear, but they are all pretty easy to make possible and document. The first two feel pretty do-able for most users that have this need.

Mr0grog avatar Dec 18 '24 03:12 Mr0grog

And another reason to do this besides dependency weight or complexity: in current versions of Node.js, @datadog/datadog-api-client causes a deprecation warning to be logged to the console. This is probably a real problem for users that aren’t just deploying this on their own servers (e.g. datadog-ci).

Mr0grog avatar Dec 18 '24 03:12 Mr0grog

Quick update: a basic implementation of this has been released as v0.13.0-pre.1. I’ve punted on the proxy issue for now, but I think it needs a solution (either built in or something like the above ideas that are easy for users to hook up).

The code in that release is also preliminary in general. Stuff I’d like to cover before a final release (and before closing this):

  • Solution for proxies.
  • Consider code organization between the DatadogReporter and Http classes.
  • Consider moving to newer Node.js than 14. Current datadog-ci versions require v18+, v20 is the oldest maintained release. We have plenty of headroom here and would still be being conservative.
  • Consider switching to ESM. We probably want to require at least Node v16 for that, though.
  • Not relevant to this issue, but just a reminder to myself: any other breaking changes we want before 1.0. The goal of v0.13 is to prepare for 1.0, with the only differences being that 1.0 drops deprecated stuff.

Mr0grog avatar Sep 01 '25 06:09 Mr0grog

TIL Node.js 24.5.0 added built-in support for proxies in its http.Agent class: https://nodejs.org/docs/latest/api/http.html#built-in-proxy-support It only reads from the environment variables if you opt-in, but from code you can always set it as needed. Of course this doesn’t help us for older Node.js versions that we support. This also doesn’t appear to work with the built-in fetch(), but it will work with cross-fetch/node-fetch.

So, some thoughts:

  • The one-size-fits-all solution is to continue depending on cross-fetch and also add proxy-agent, using it as the agent option with cross-fetch’s fetch(). Pretty simple! Just works.

    • A slightly less nice version where we make proxy-agent an optional dependency, and if you try to configure a proxy without it installed, we log a warning or maybe an error.
    • Supports proxy env vars out of the box in all environments.
    • Could add a proxy option (that is just a URL string) if we wanted. Maybe skip for now, leave the door open if somebody makes a feature request.
    • No need to implement a way for people to provide a custom HTTP/fetch implementation.
  • The middle-of-the-road solution is to continue with cross-fetch, and include automatic proxy support on Node.js 24.5+, Deno, and Bun.

    • Need to implement one of the approaches above for providing a custom HTTP/fetch implementation as a fallback for older Node.js or other environments.
    • A little complicated because we need to check if we are in Deno or Bun and use the native fetch (with the appropriate options) before we try to use cross-fetch’s fetch.
    • Could add a proxy option (that is just a URL string) if we wanted. Maybe skip for now, leave the door open if somebody makes a feature request.
  • The minimal solution would be to drop cross-fetch in favor of the built-in for all environments.

    • Built-in support for proxy env vars in Deno and Bun, and in Node.js 24.5+ if users also set the NODE_USE_ENV_PROXY=1 environment variable.
    • We wouldn’t provide any library-level proxy configuration since it would be so limited (only Deno and Bun) — just the env vars.
    • Need to implement one of the approaches above for providing a custom HTTP/fetch implementation as a fallback for older Node.js or other environments.

Now that I've written it out, “middle-of-the-road” from the usage perspective is a lot of work on our side!

Leaning toward “one-size-fits-all,” but with proxy-agent as an optional dependency. If env vars are set and it’s not installed, log a warning. If library-level config is set and it’s not installed, throw an error.

My biggest concern with that is exposing the optional dependency on proxy-agent makes it hard to drop cross-fetch/node-fetch in the future, since it’s not the right solution for Node’s built-in fetch (node-fetch hasn’t had any updates in over 2 years; it may be on its way out).

Mr0grog avatar Sep 11 '25 00:09 Mr0grog