node-datadog-metrics
node-datadog-metrics copied to clipboard
Consider dropping `@datadog/datadog-api-client`?
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!).
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).
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.
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-fetchis a little complicated because it is a placeholder for whateverfetchis 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’sfetchsupports anagentoption, which works like the oldagentoption we used to support when we were based ondogApi.) - In other environments, you get Node-fetch, which takes an
agentoption like we used to support. Pretty straightforward. - Need to do more checking to find out what happens if we supply
agentto an underlying implementation that does not support it, or if we can detect what underlying implementation we wound up with and supplyagentif supported.
node-fetchtakes anagentoption we can supply or pass through. See above.axiostakes anhttpsAgentoption like node-fetch’sagent, or takes aproxyobject with various options.undicitakes adispatcheroption that can be an instance ofProxyAgent(which it provides).gottakes anagentoption (more-or-less like node-fetch).- Node.js’s built-in
fetchis based on Undici but does not support thedispatcheroption 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
fetchtakes aproxyoption that is a string of the same format you’d normally provide theHTTP_PROXYorHTTPS_PROXYenvironment variable for lots of CLI tools. - Deno’s
fetchtakes aclientoption 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-agentfor this not-super-common case and - I think exposing an under-the-hood thing like
agentin 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’sagentoption available.
- I’d prefer not to also include
- 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.
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).
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
DatadogReporterandHttpclasses. - 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.
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
agentoption with cross-fetch’sfetch(). 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
proxyoption (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
proxyoption (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=1environment 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.
- Built-in support for proxy env vars in Deno and Bun, and in Node.js 24.5+ if users also set the
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).