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

Allow to use a fully custom datadog ingestion endpoint #107

Closed
wants to merge 3 commits into from

Conversation

PerGon
Copy link

@PerGon PerGon commented Dec 12, 2022

Under some circumstances it is useful to have metrics shipped elsewhere than datadog endpoints. This is also supported in other Datadog applications (e.g. Datadog Agent).

This PR adds a new configuration parameter which will allow to set a fully custom server url. This will allow to ship the metrics to that server, as opposed to the hardcoded values defined in the @datadog/datadog-api-client library.

Copy link
Collaborator

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an actual use case you currently need this for, and that can’t be accomplished just a well with a custom reporter? For example:

metrics.init({
  reporter: {
    report(series, onSuccess, onError) {
      // Send metrics wherever, however you want here
    }
  }
});

Even though it’s not a huge change…

  • It adds a fair amount of conceptual complexity, and can be confusing when and why you should use it vs. apiHost (e.g. “why not set customServerUrl: 'api.datadoghq.eu?).
  • It also represents new risks for breaking changes — addressing issues like Any interest in adding events? #12 or supplying units/metric metadata #33 become more complex (will other APIs people are using this package for support the new formats and endpoints? etc.).

After all, the goal of this package is simplify using DataDog. People with complex needs can always use @datadog/datadog-api-client directly.

lib/reporters.js Outdated Show resolved Hide resolved
lib/reporters.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/reporters.js Outdated Show resolved Hide resolved
@PerGon
Copy link
Author

PerGon commented Dec 14, 2022

Thank you for the review and the comments. I've pushed some changes that address them 🤞

Do you have an actual use case you currently need this for, and that can’t be accomplished just a well with a custom reporter?

Our organisation is currently migrating away from Datadog. One of the tools we are replacing Datadog with, does support the ingestion of Datadog metrics format (e.g.) - this is useful as it allows to migrate from Datadog without significant changes in thousands of services, apps, jobs, lambdas, etc. (which would be a very significant engineering effort).

After years of using Datadog, we use a wide range of libraries to send metrics to Datadog. Luckily, most of them are customisable to the point where we can define a non-datadog URL to send metrics to. Even the datadog provided libraries and agents allow for such (server agent, kubernetes agent, various libraries (e.g. python)).
We have already migrated dozens of services and applications by using this method.

metrics.init({
  reporter: {
    report(series, onSuccess, onError) {
      // Send metrics wherever, however you want here
    }
  }
});

From my understanding, this does not allow to send to a fully customisable URL. But I might be missing the right config on how to do it.

It adds a fair amount of conceptual complexity, and can be confusing when and why you should use it vs. apiHost (e.g. “why not set customServerUrl: 'api.datadoghq.eu?).

I tried to use the apiHost, but it didn't really work very well. For these reasons:

  • There was already a transformation being done one the value of this variable. I was looking for a way where the user as full control.
  • apiHost uses the Datadog config called site which in the Datadog library implies a Datadog site and where additional transformations to the URL are done - it forces https (which I know it's a good idea, but we have some internal traffic not yet using https 🤷‍♂️ ), it prefixes subdomains, etc.

I understand that the usage of apiHost and customServerUrl can get confusing, but hopefully the above explanation shed some light on the use-case.

And yes, in extremis, customServerUrl: https://api.datadoghq.eu is equivalent to apiHost: datadoghq.eu. But not the other way around. If I set apiHost: insert-metrics-here.com/datadog metrics will be shipped here https://api.insert-metrics-here.com/datadog (not really where I want them).

It also represents new risks for breaking changes — addressing issues like #12 or #33 become more complex (will other APIs people are using this package for support the new formats and endpoints? etc.).

Indeed. I think this feature can be seen as an advanced feature. Maybe it can be stated that it's up to the server defined in customServerURL to implement all the relevant endpoints for the proper ingestion of metrics/events/etc.

After all, the goal of this package is simplify using DataDog. People with complex needs can always use @datadog/datadog-api-client directly.

I understand and that is one of the reasons this package is used in various projects 😃
If you thing this creates more confusion than the benefit it adds or it goes against the ethos of the project, no hurt feelings, I'll be happy to close the PR unmerged.

In any case, thanks for the help. Well, most of all, many thanks for the package 🙂

@Mr0grog
Copy link
Collaborator

Mr0grog commented Dec 15, 2022

From my understanding, [a custom reporter] does not allow to send to a fully customisable URL. But I might be missing the right config on how to do it.

It should let you do it! The reporter is what actually sends data to the API, so it can ignore all the other settings about URLs and send the data wherever it wants. The default setup is equivalent to this:

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

And the actual internals of DataDogReporter are effectively pretty similar to this, which you could do:

metrics.init({
  reporter: {
    report(series, onSuccess, onError) {
      // `series` is an array of metrics objects, formatted basically how the
      // DD v1 metrics API and v1 distributions API want them.
      // This function is responsible for sending the metrics and reporting
      // success or failure.
      // 
      // This is a *simplified* example of what sending to the DD API is
      // basically doing, and you might do similar with different URLs or
      // auth or formatting for the body:
      fetch('https://my-datadog-like-api.com/api/v1/series', {
          method: 'POST',
          headers: {
            'Accept': 'application/json',
            'Content-Type': 'text/json',
            'DD-API-KEY': 'YOUR_API_KEY'
          },
          // Format the metrics 
          body: JSON.stringify({ series })
        })
          .then(response => response.json())
          .then(() => onSuccess())
          .catch(onError);
    }
  }
});

(One thing that is more complicated than the above example: “distributions” go to a different API endpoint than all other metrics, so in practice we make two HTTP requests instead of one. A different metrics service might not be the same.)

Obviously, that’s a little more complicated for a user to implement than just adding a configuration variable like you are proposing here. But it is more flexible, and lets you migrate to different APIs and different formats that aren’t similar to Datadog at all. (For example, if the service you are migrating to has a more purpose-built API that you want to use instead of the Datadog-like one.)

I tried to use the apiHost, but it didn't really work very well. For these reasons…

I’m sorry; I explained poorly. I understand what it’s doing; I meant to say that I don’t think this is entirely clear to new users of this library who are looking for a quick & simple way to send metrics. In other words, an explanation of this needed to be in the docs. I think you already improved that a fair bit in your last commit. Maybe linking to an example service that supports the DataDog API (like VictoriaMetrics from your comment above) would be good, too.

Maybe it can be stated that it's up to the server defined in customServerURL to implement all the relevant endpoints for the proper ingestion of metrics/events/etc.

👍 please add to the docs!

Given that you actually need this for work you are trying to do, I think it will probably be OK to merge (unless the flexibility of a custom reporter actually serves you better, in which case this isn’t worthwhile!). I’m most concerned about making it clear to users what this does or doesn’t do.

@PerGon
Copy link
Author

PerGon commented Dec 15, 2022

Thank you very much for your detailed explanation.

After some back and forth, I was able to get it to work by using your suggestion from the last comment. Ended up with something like this:

  const customReporter = {
    report(series, onSuccess, onError) {
      fetch('https://my-metrics-insert-endpoint.com/insert/1/datadog/api/v1/series', {
        method: 'POST',
        headers: {
          'Accept': 'application/json',
          'Content-Type': 'text/json',
          'DD-API-KEY': 'DUMMY_KEY',
        },
        // Format the metrics
        body: JSON.stringify({series}),
      })
          .then((response) => response.json())
          .then(onSuccess)
          .catch(onError);
    },
  };

  const ddClient = new metrics.BufferedMetricsLogger({
    reporter: customReporter,
    prefix: METRICS_NAMESPACE,
    flushIntervalSeconds: 1,
    defaultTags: ['service:myService'],
  });
  

From my understanding this is equivalent to your suggestion.

As far as I can tell, we are not using distributions, so this way will serve us well. One problem at the time 😄

Given that you actually need this for work you are trying to do, I think it will probably be OK to merge (unless the flexibility of a custom reporter actually serves you better, in which case this isn’t worthwhile!). I’m most concerned about making it clear to users what this does or doesn’t do.

Really appreciate the open-mindedness, but I think you got us covered! In the meantime, I'll close the PR.

Once again, thanks for the help 🙇

@PerGon PerGon closed this Dec 15, 2022
@Mr0grog
Copy link
Collaborator

Mr0grog commented Dec 15, 2022

Great, I’m glad this helped! It sounds like we could at least improve the docs to describe custom reporters in more detail, so there’s still some improvements to make based on this. :)

Mr0grog added a commit that referenced this pull request Feb 21, 2023
The proposed feature in #107 was withdrawn because the problem it was solving could have already been handled with a custom reporter. This adds some documentation about how to set up a custom reporter to prevent similar misunderstandings in the future.
ErikBoesen pushed a commit that referenced this pull request Feb 21, 2023
The proposed feature in #107 was withdrawn because the problem it was solving could have already been handled with a custom reporter. This adds some documentation about how to set up a custom reporter to prevent similar misunderstandings in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants