Feat: Support OTel tracing using ngx_otel_module #1238

ciarams87 · 2023-11-09T18:04:37Z

Proposed changes

Problem:
As a user of NGF
I want NGF to have the ability to generate and send traces
So that I can easily get tracing for all of my traffic to help debug my environment.

Solution: Use the NGINX Otel module to configure tracing from NGINX. Use the NginxProxy CRD to allow the user to configure tracing.

Testing: Unit testing, and e-2-e testing locally using Jaegar on Kind

Please focus on: This is based on a feature branch, to allow for someone else to also start work on #483. I could merge into the feature branch instead if required, but for now I am targeting merging to main as so far no work has happened yet on that issue.

Closes #1166

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

build/Dockerfile.nginx

sjberman · 2023-11-09T19:42:54Z

apis/v1alpha1/nginxproxy_types.go

+	// +optional
+	// +kubebuilder:default="5s"
+	// +kubebuilder:validation:Pattern=`^(\d+y)??\s*(\d+M)??\s*(\d+w)??\s*(\d+d)??\s*(\d+h)??\s*(\d+m)??\s*(\d+s?)??\s*(\d+ms)??$`
+	Interval string `json:"interval,omitempty"`


optional fields should be pointers

sjberman · 2023-11-09T19:46:50Z

apis/v1alpha1/nginxproxy_types.go

+	//
+	// +optional
+	// +kubebuilder:default=512
+	BatchSize int `json:"batchSize,omitempty"`


I believe k8s recommends integer types be int32, so they aren't ambiguous in their size.

build/Dockerfile.nginx

sjberman · 2023-11-09T19:58:18Z

internal/mode/static/state/graph/nginxproxy.go

+			tracing := ngfproxy.Spec.HTTP.Telemetry.Tracing
+			proxy.Tracing = &Tracing{
+				Endpoint:    tracing.Endpoint,
+				ServiceName: gwcName + ":ngf",


What if we make this the Gateway name instead of the class name? Thinking ahead to having multiple Gateways per Class, may want to delineate between them better.

perhaps Gateway Namespace and Name, not just name?

sjberman · 2023-11-09T20:36:46Z

deploy/helm-chart/values.yaml

@@ -58,6 +58,9 @@ nginx:
    tag: edge
    pullPolicy: Always

+  ## The configuration for the data plane that is contained in the NginxProxy resource.
+  config: {}


To make it easier on users to understand the structure, maybe we add the telemetry config in here but comment it out. Similar to how we did it in the old NSM chart

apis/v1alpha1/nginxproxy_types.go

docs/data-plane-configuration.md

internal/mode/static/state/graph/gatewayclass.go

internal/mode/static/nginx/config/tracing_template.go

internal/mode/static/nginx/config/tracing_test.go

pleshakov · 2023-11-09T22:42:47Z

internal/mode/static/nginx/config/tracing_template.go

@@ -0,0 +1,12 @@
+package config
+
+var otelTemplateText = `


should we use a const?

We use var for the other <x>TemplateText, does it make sense to use const just here or everywhere else too?

yeah, const makes sense. not sure why we needed a var

sjberman · 2023-11-10T00:12:15Z

apis/v1alpha1/nginxproxy_types.go

+	//
+	// +optional
+	// +kubebuilder:default=false
+	Enabled bool `json:"enabled,omitempty"`


Now that I think about it, this should probably be "enable" present tense to match our other enable/disable fields.

kate-osborn · 2023-11-10T19:19:50Z

docs/data-plane-configuration.md

+| --------- | ---------------------------------------------- | --------------------------- | -------- |
+| telemetry | Telemetry defines the telemetry configuration. | [telemetry](#spectelemetry) | no       |
+
+### Spec.Telemetry


Should this be Spec.HTTP.Telemetry?

If so, would it need to be Spec.HTTP.Telemetry.Tracing? But I also see there are parts where we just reference proxy.Tracing without needing to go through the objects.

If so, would it need to be Spec.HTTP.Telemetry.Tracing

Yes, I've updated it 👍🏼

But I also see there are parts where we just reference proxy.Tracing without needing to go through the objects.

I think what you're referring to is when we're referencing the graph Tracing object?

Yea, it's in the configuration.go files I'm seeing the graph's NginxProxy directly reference proxy.Tracing, why is that able to do that?

kate-osborn · 2023-11-10T19:23:56Z

examples/tracing/README.md

@@ -0,0 +1,190 @@
+# Example
+
+In this example we deploy NGINX Gateway Fabric, a simple web application, configure NGINX Gateway Fabric


I'm finding this a bit hard to follow. I wonder if we need to describe everything we are going to do in the example or if it's enough to describe the tracing piece of it.

Yeah I struggled a bit with this example 😅 I simplified the description, but maybe I should change it completely remove the deploy app/ gateway/ HTTPRoutes? Just describe how to configure the tracing?

Is documentation enough? Is there anything specific that would be in the example that wouldn't be in our docs?

Yeah, that's a good point. Happy to remove it if we don't feel it adds value.

I personally like this example because it's an easy step-by-step guide a user can follow. Also, it makes it easy for us to test tracing.

@ciarams87 I think the new intro is much better 👍

kate-osborn · 2023-11-10T19:25:25Z

examples/tracing/README.md

+1. Get the local DNS name of the all in one Jaegar collector service. It will follow the standard
+   [Kubernetes DNS naming conventions](https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#a-aaaa-records)
+   e.g. `simplest-collector.default.svc.cluster.local`.
+   Also get the number of either the `grpc-otlp` port or the `http-otlp` port of the collector service, e.g.


nit: comma after "Also"

kate-osborn · 2023-11-10T19:26:49Z

examples/tracing/README.md

+   ```
+
+
+## 2. Deploy the Cafe Application


This should be step 3

kate-osborn · 2023-11-10T19:41:51Z

internal/mode/static/state/graph/gatewayclass.go

@@ -76,7 +75,7 @@ func buildGatewayClass(gc *v1beta1.GatewayClass, npCfg *ngfAPI.NginxProxy) *Gate
 	}
 }

-func validateGatewayClass(gc *v1beta1.GatewayClass, npCfg *ngfAPI.NginxProxy) error {
+func validateGatewayClass(gc *v1beta1.GatewayClass, npCfg *NginxProxy) error {
 	if gc.Spec.ParametersRef != nil {


A related question about validation: If tracing is disabled, but a tracing endpoint is provided, what should we do?

kate-osborn · 2023-11-10T19:44:08Z

internal/mode/static/state/graph/nginxproxy.go

@@ -7,18 +7,59 @@ import (
 	ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
 )

+type NginxProxy struct {


I'm probably missing something, but why do we need these NginxProxy and Tracing struct definitions? Can't we use the ones from ngfAPI here?

I wanted to simplify where we had to do the nested nil-checking, but also now with the validation code, I think it makes sense to have this is a separate type. What do you think?

I see two pros for removing it:

for consistency with other types - we don't do the same thing for other types

although we will need to do nil checking, we're not gonna do it twice - we can convert NginxProxy into dataplane.Tracing when building dataplane.Configuration

My concern is around validation and building the status. I'd need to do something different here than what we do for the other resources, because I'm doing the validation here and need to propagate the conditions to the status updater.

lucacome · 2023-11-10T20:00:59Z

build/Dockerfile.nginx

@@ -4,11 +4,15 @@ FROM nginx:1.25.3-alpine
 ARG NJS_DIR
 ARG NGINX_CONF_DIR

-RUN apk update && apk upgrade && apk add --no-cache libcap \
+RUN apk update && apk upgrade && apk add --no-cache libcap curl ca-certificates \
+    && printf "%s%s%s\n" "http://nginx.org/packages/mainline/alpine/v" `egrep -o '^[0-9]+\.[0-9]+' /etc/alpine-release` "/main" | tee -a /etc/apk/repositories \


Suggested change

&& printf "%s%s%s\n" "http://nginx.org/packages/mainline/alpine/v" `egrep -o '^[0-9]+\.[0-9]+' /etc/alpine-release` "/main" | tee -a /etc/apk/repositories \

&& printf "%s\n" "http://nginx.org/packages/mainline/alpine/v$(egrep -o '^[0-9]+\.[0-9]+' /etc/alpine-release)/main" >> /etc/apk/repositories \

This seems cleaner to me 😄 (but anyway we don't need tee)

I took this from the official NGINX docs, but yes, yours looks a lot cleaner 😂

sjberman · 2023-11-14T15:19:53Z

Be sure to update the proposal for the data plane config to "Completed".

bjee19 · 2023-11-14T19:09:05Z

deploy/helm-chart/crds/gateway.nginx.org_nginxproxies.yaml

+            description: NginxProxySpec defines the desired state of the NginxProxy.
+            properties:
+              http:
+                description: Http defines the Http configuration.


If we make the change above that @pleshakov suggested about Http -> HTTP, should change here too.

bjee19 · 2023-11-14T19:09:28Z

deploy/manifests/crds/gateway.nginx.org_nginxproxies.yaml

+            description: NginxProxySpec defines the desired state of the NginxProxy.
+            properties:
+              http:
+                description: Http defines the Http configuration.


Same comment from other CRD yaml about Http -> HTTP

bjee19 · 2023-11-14T19:12:24Z

docs/data-plane-configuration.md

+| --------- | ---------------------------------------------- | --------------------------- | -------- |
+| telemetry | Telemetry defines the telemetry configuration. | [telemetry](#spectelemetry) | no       |
+
+### Spec.Telemetry


If so, would it need to be Spec.HTTP.Telemetry.Tracing? But I also see there are parts where we just reference proxy.Tracing without needing to go through the objects.

sjberman · 2023-11-15T19:28:44Z

apis/v1alpha1/nginxproxy_types.go

+	HTTP *HTTP `json:"http,omitempty"`
+}
+
+// HTTP defines the HTTP configuration.


Can we be more explicit to say that this is for the nginx http block configuration?

sjberman · 2023-11-15T19:30:40Z

apis/v1alpha1/nginxproxy_types.go

+	//
+	// +optional
+	// +kubebuilder:default="5s"
+	// +kubebuilder:validation:Pattern=`^(\d+y)??\s*(\d+M)??\s*(\d+w)??\s*(\d+d)??\s*(\d+h)??\s*(\d+m)??\s*(\d+s?)??\s*(\d+ms)??$`


Is this the same validation pattern that the otel module uses?

sjberman · 2023-11-15T19:31:14Z

apis/v1alpha1/nginxproxy_types.go

+	// BatchSize specifies the maximum number of spans to be sent in one batch per worker. Default is 512.
+	//
+	// +optional
+	// +kubebuilder:default=512


Let's set validation to not allow a negative value. Not sure if we also should set a max?

sjberman · 2023-11-15T19:31:28Z

apis/v1alpha1/nginxproxy_types.go

+	// BatchCount specifies the number of pending batches per worker, spans exceeding the limit are dropped. Default is 4.
+	//
+	// +optional
+	// +kubebuilder:default=4


Same comment as previous.

sjberman · 2023-11-15T19:31:56Z

apis/v1alpha1/nginxproxy_types.go

+	//
+	// +required
+	// ++kubebuilder:validation:Required
+	// ++kubebuilder:validation:Pattern=`^(?:(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}|(?:\d{1,3}\.){3}\d{1,3}):\d{1,5}$`


Same question as before about the pattern.

are IPv6 addresses allowed?

sjberman · 2023-11-15T19:35:28Z

docs/data-plane-configuration.md

+
+The control plane only watches this single instance of the custom resource. If the resource is invalid per the OpenAPI
+schema, the Kubernetes API server will reject the changes. If the resource is deleted or deemed invalid by NGINX
+Gateway Fabric, a warning Event is created in the `nginx-gateway` Namespace, and the default values will be used by


Do we actually create an event for this?

sjberman · 2023-11-15T19:36:12Z

docs/data-plane-configuration.md

+of the resource is `<release-name>-proxy-config`. It is deployed in the same Namespace as the controller
+(default `nginx-gateway`).
+
+The control plane only watches this single instance of the custom resource. If the resource is invalid per the OpenAPI


Rather than "only watches this single instance of the custom resource", I'd mention that we only watch the one that's referenced in the associated GatewayClass for this control plane.

sjberman · 2023-11-15T19:36:41Z

docs/data-plane-configuration.md

+
+| name       | description                                                                                                         | type   | required |
+| ---------- | ------------------------------------------------------------------------------------------------------------------- | ------ | -------- |
+| enable    | Enabled enables or disables OpenTelemetry tracing at the HTTP context. Default is false.                            | bool   | no       |


sjberman · 2023-11-15T19:38:10Z

docs/data-plane-configuration.md

+
+```shell
+kubectl -n nginx-gateway describe nginxproxies nginx-proxy-config
+```


Per my other comment regarding whether or not we need an example, I would update this doc maybe with an example Jaeger deployment and telemetry config that would send traces there.

internal/mode/static/state/graph/nginxproxy.go

pleshakov · 2023-11-15T21:04:22Z

apis/v1alpha1/nginxproxy_types.go

+	//
+	// +required
+	// ++kubebuilder:validation:Required
+	// ++kubebuilder:validation:Pattern=`^(?:(?:[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}|(?:\d{1,3}\.){3}\d{1,3}):\d{1,5}$`


are IPv6 addresses allowed?

pleshakov · 2023-11-15T21:11:21Z

internal/mode/static/nginx/conf/nginx.conf

@@ -1,4 +1,5 @@
 load_module /usr/lib/nginx/modules/ngx_http_js_module.so;
+load_module modules/ngx_otel_module.so;


We load this file into the base image, do you think we need to create a new includes at this level for global configuration options like this?

yeah. that could work

pleshakov · 2023-11-15T21:19:07Z

apis/v1alpha1/nginxproxy_types.go

+// +kubebuilder:storageversion
+// +kubebuilder:subresource:status
+
+// NginxProxy represents the dynamic configuration for an NGINX Gateway Fabric data plane.


Suggested change

// NginxProxy represents the dynamic configuration for an NGINX Gateway Fabric data plane.

// NginxProxy represents the configuration for an NGINX Gateway Fabric data plane.

pleshakov · 2023-11-15T22:03:20Z

internal/mode/static/nginx/config/tracing_template.go

@@ -0,0 +1,12 @@
+package config
+
+var otelTemplateText = `


yeah, const makes sense. not sure why we needed a var

pleshakov · 2023-11-15T22:40:14Z

internal/mode/static/state/graph/nginxproxy.go

+	if ngfTracing.Interval != nil && !reTime.MatchString(*ngfTracing.Interval) {
+		valErr = append(valErr, field.Invalid(
+			path.Child("interval"), *ngfTracing.Interval, "tracing.interval must be a valid time string"))
+	}


shall we also validate that BatchCount and BatchSize are not negative?

sjberman · 2023-11-16T17:16:10Z

There are probably some v1beta1 references that were added that will need to be updated to v1 once #1250 is merged and this branch is rebased.

Problem: We have two upcoming stories that require the new NginxProxy CRD in order to be worked on. Solution: To prevent one story from being blocked by the other, this work adds the new CRD with a blank spec, a controller that watches for it and triggers an update, and sets the status on the resource.

ciarams87 · 2023-11-20T09:28:38Z

Closed as we are taking a different direction - see #1258

ciarams87 requested a review from a team as a code owner November 9, 2023 18:04

github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart labels Nov 9, 2023

ciarams87 commented Nov 9, 2023

View reviewed changes

build/Dockerfile.nginx Outdated Show resolved Hide resolved

ciarams87 force-pushed the feat/support-otel-tracing branch 2 times, most recently from 6fcef3d to 9c11e1e Compare November 9, 2023 18:51

sjberman reviewed Nov 9, 2023

View reviewed changes

pleshakov reviewed Nov 9, 2023

View reviewed changes

sjberman reviewed Nov 10, 2023

View reviewed changes

kate-osborn requested changes Nov 10, 2023

View reviewed changes

lucacome reviewed Nov 10, 2023

View reviewed changes

bjee19 reviewed Nov 14, 2023

View reviewed changes

ciarams87 force-pushed the feat/support-otel-tracing branch from 623a8e1 to a510628 Compare November 15, 2023 16:12

ciarams87 requested review from kate-osborn, bjee19, sjberman, pleshakov and lucacome November 15, 2023 16:39

sjberman reviewed Nov 15, 2023

View reviewed changes

pleshakov reviewed Nov 15, 2023

View reviewed changes

sjberman and others added 4 commits November 17, 2023 11:00

Add tracing support

e754e5c

Review feedback; add validation

07d753f

Update data plane config proposal && fix rebase issues

b806b53

ciarams87 force-pushed the feat/support-otel-tracing branch from 800bad7 to b806b53 Compare November 17, 2023 11:07

Add in progress review feedback

87da9e2

ciarams87 closed this Nov 20, 2023

		@@ -0,0 +1,190 @@
		# Example

		In this example we deploy NGINX Gateway Fabric, a simple web application, configure NGINX Gateway Fabric

	&& printf "%s%s%s\n" "http://nginx.org/packages/mainline/alpine/v" `egrep -o '^[0-9]+\.[0-9]+' /etc/alpine-release` "/main" \| tee -a /etc/apk/repositories \
	&& printf "%s\n" "http://nginx.org/packages/mainline/alpine/v$(egrep -o '^[0-9]+\.[0-9]+' /etc/alpine-release)/main" >> /etc/apk/repositories \

	\| enable \| Enabled enables or disables OpenTelemetry tracing at the HTTP context. Default is false. \| bool \| no \|
	\| enable \| Enable enables or disables OpenTelemetry tracing at the HTTP context. Default is false. \| bool \| no \|

		@@ -1,4 +1,5 @@
		load_module /usr/lib/nginx/modules/ngx_http_js_module.so;
		load_module modules/ngx_otel_module.so;

	// NginxProxy represents the dynamic configuration for an NGINX Gateway Fabric data plane.
	// NginxProxy represents the configuration for an NGINX Gateway Fabric data plane.

Feat: Support OTel tracing using ngx_otel_module #1238

Feat: Support OTel tracing using ngx_otel_module #1238

Conversation

ciarams87 commented Nov 9, 2023 • edited Loading

Proposed changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjee19 Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjberman commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleshakov Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleshakov Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjberman commented Nov 16, 2023

ciarams87 commented Nov 20, 2023

ciarams87 commented Nov 9, 2023 •

edited

Loading

bjee19 Nov 15, 2023 •

edited

Loading

pleshakov Nov 15, 2023 •

edited

Loading

pleshakov Nov 15, 2023 •

edited

Loading