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

Consider dropping @datadog/datadog-api-client? #132

Open
Mr0grog opened this issue Nov 12, 2024 · 4 comments
Open

Consider dropping @datadog/datadog-api-client? #132

Mr0grog opened this issue Nov 12, 2024 · 4 comments

Comments

@Mr0grog
Copy link
Collaborator

Mr0grog commented Nov 12, 2024

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
Copy link
Collaborator Author

Mr0grog commented Nov 28, 2024

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 Mr0grog added this to the v0.13.0 milestone Dec 6, 2024
@Mr0grog
Copy link
Collaborator Author

Mr0grog commented Dec 6, 2024

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
Copy link
Collaborator Author

Mr0grog commented Dec 18, 2024

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
Copy link
Collaborator Author

Mr0grog commented Dec 18, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant