DataDog / nginx-datadog

Enhance NGINX Observability and Security with Datadog's Module
https://www.datadoghq.com
Apache License 2.0
23 stars 9 forks source link

Enable by condition #20

Closed fabfuel closed 1 year ago

fabfuel commented 1 year ago

Hi there,

how can I enable the module conditionally? I want to be able to control, if the module is enabled, because we don't run Datadog agent locally or during CI.

The API.md says, that datadog_enable can be used in if context, but I was not able to make it work, the datadog_enable directive is just ignored in any if (...) {} block, like for example:

map $host $with_datadog {
    default   $DATADOG_ENABLED;
}

server {
    add_header X-Debug $with_datadog;

    if ($with_datadog = "true") {
        datadog_enable;
    }

    ...

But it works well without the if block

server {
    datadog_enable;

    ...

It's not an issue with the env var, I use envsubst and the add_header works as expected, the response header is true

X-Debug: true

Thankful for any hint!

Best Fabian

dgoffredo commented 1 year ago

datadog_enable is the default behavior. Try conditionally invoking datadog_disable instead.

There's no test for using datadog_disable in an if context. I'll see about adding one.

dgoffredo commented 1 year ago

I just wrote a test: it doesn't work.

load_module modules/ngx_http_datadog_module.so;

events {
    worker_connections  1024;
}

http {
    server {
        listen       80;

        if ($http_x_with_tracing = "false") {
            datadog_disable;
        }

        location /http {
            proxy_pass http://http:8080;
        }
    }
}

This will add tracing to the /http endpoint no matter what the value of the x-with-tracing request header.

Now that I think about it, it's not surprising. The if directive is part of the rewrites module, while datadog_disable takes effect only at configuration time -- it has no way of knowing the outcome of the if, because it's not something that happens per-request. Even in your example, where the value of the predicate is known at startup, the if block really is not a conditional configuration construct.

I should probably remove support for those directives in if context.

That aside, let's talk about your problem. You want to trace in production but not in CI, is that right? Maybe we can find a workaround that doesn't involve if.

dgoffredo commented 1 year ago

I could add variants of datadog_enable and/or datadog_disable (or an appropriately named third directive) that takes an expression argument, e.g.

server {
    ...
    datadog_enable_if "some $variable expression";
    ...
}

The tracing implementation could then evaluate the expression at every request, and conditionally enable/disable tracing.

I'd prefer not to implement this if there is a way for us to set up your configuration without it.

fabfuel commented 1 year ago

Thanks for the quick reply @dgoffredo!

You want to trace in production but not in CI, is that right?

Yes, exactly!

The variant you proposed would bring great flexibility, but is not necessary for my use-case. I only need a main switch for globally enabling or disabling tracing via env var.

I was wondering, if tracing could be completely disabled, if e.g. no agent host is configured. One error message at startup would be no problem, but currently I get six error logs for every single request, in local or CI setups without the agent running or not properly configured.

nginx  | 2023/03/21 15:02:27 [error] 93#95: datadog: Error sending traces to agent: Couldn't connect to server
nginx  | Failed to connect to localhost port 8126: Connection refused
nginx  | 2023/03/21 15:02:27 [error] 93#95: datadog: Error sending traces to agent: Couldn't connect to server
nginx  | Failed to connect to localhost port 8126: Connection refused
nginx  | 2023/03/21 15:02:27 [error] 93#95: datadog: Error sending traces to agent: Couldn't connect to server
nginx  | Failed to connect to localhost port 8126: Connection refused

Best Fabian

dgoffredo commented 1 year ago

In your example above:

map $host $with_datadog {
    default   $DATADOG_ENABLED;
}

server {
    add_header X-Debug $with_datadog;

    if ($with_datadog = "true") {
        datadog_enable;
    }

    ...

Is $DATADOG_ENABLED a placeholder for demonstrative purposes, or are you using something like envsubst to pull it in from the environment?

If you're doing the latter, then here's hack that can work:

server {
    ...
    $DATADOG_ENABLEMENT_DIRECTIVE;

    location /foo {
        ...
    }
}
$ export DATADOG_ENABLEMENT_DIRECTIVE=datadog_enable
$ >nginx.prod.conf envsubst '$DATADOG_ENABLEMENT_DIRECTIVE' <nginx.template.conf
$ export DATADOG_ENABLEMENT_DIRECTIVE=datadog_disable
$ >nginx.ci.conf envsubst '$DATADOG_ENABLEMENT_DIRECTIVE' <nginx.template.conf

Hardly better than sed, though.

fabfuel commented 1 year ago

Yes, I'm using envsubst, it's an env var. I will try your hack and let you know!

fabfuel commented 1 year ago

It works 🎉 Thanks for the hack @dgoffredo 🙂

dgoffredo commented 1 year ago

Glad to hear it!

I'm going to leave this open for the work of either updating the module, its documentation, or providing a better solution.

dgoffredo commented 1 year ago

The next release will remove support for datadog_* directives in "if context," so this is a "fix" via "will not support."

Please open up another issue if you encounter further problems or have ideas about how better to support multiple configuration environments.