grafana / alloy

OpenTelemetry Collector distribution with programmable pipelines
https://grafana.com/oss/alloy
Apache License 2.0
1.28k stars 172 forks source link

Nginx Metrics Exporter #237

Open CSalih opened 6 months ago

CSalih commented 6 months ago

Request

Propose for adding functionality to the Grafana Agent to enable the collection of Nginx metrics using the nginx-prometheus-exporter (https://github.com/nginxinc/nginx-prometheus-exporter).

This feature is essential for users relying on Nginx as their web server, allowing them to monitor Nginx performance metrics within Grafana.

Use case

This feature offers users to monitor, analyze, and optimize their Nginx servers within the Grafana ecosystem. As Nginx being a widely adopted web server this may be a relevant.

tpaschalis commented 6 months ago

Hey there Salih 👋

Yes, sounds like a good addition. Since the exporter is already written in Go, this sounds like it's a straightforward thing.

Would you be interested in contributing as a Flow component? You can look at the list of existing exporter components and use one for reference (eg. prometheus.exporter.apache](https://github.com/grafana/agent/blob/main/component/prometheus/exporter/apache/apache.go).

You might have to bring some of the code directly into the new package (instead of putting it on pkg/integrations/apache_http like Apache does), but it should be pretty straightforward and we'd love to help you get started!

CSalih commented 6 months ago

Hi Paschalis,
I would love to contribute to this project. The next weeks I'm a bit busy, but I'll give my best to start next week. It seems pretty straightforward maybe I can implement it very quickly :fast_forward:. Thank you very much for offering to help me, I will get back to you if I need help.

CSalih commented 6 months ago

@tpaschalis, the implementation was quicker than I expected (https://github.com/CSalih/grafana-agent/tree/feat/nginx-collector).

But before I create the PR, i do have some questions:

You might have to bring some of the code directly into the new package

Grafana Cloud Integrations still uses static mode, so we might need this (at least I would need it).

Q: Where shell I put the static mode code? Into /component/prometheus/exporter/nginx and import it in pkg/integrations/install/install.go or is there a way to "convert" it from flow mode? Currently I did put it, like apache, in pkg/integrations/nginx_http.

Q: I did not add any test. Would it be nice to have unit test, should i add some?

Q: Should I create a single PR or split it into multiple one (e.g. one for static and one for flow)?

jkroepke commented 6 months ago

I may ask myself, if it's worth to have https://github.com/nginxinc/nginx-prometheus-exporter on board. While the exporter produce 8 metrics for the open-source variant, 129 metrics are available for nginx+ (commercial product) only.

Currently, we are using log based alternatives, where the nginx logs are sent via syslog protocol to exporters.

But exporters proving much more information for nginx as https://github.com/nginxinc/nginx-prometheus-exporter it does.

Q: Should I create a single PR or split it into multiple one (e.g. one for static and one for flow)?

In history, I do both in one single PR.

tpaschalis commented 6 months ago

Hey @CSalih, apologies for the delayed response!

Q: Where shell I put the static mode code? Currently I did put it, like apache, in pkg/integrations/nginx_http.

If you wanted to also add the static mode version, then yes, please follow the apache_http example, add it under pkg/integrations/nginx_http and then import this from the component definition.

Q: I did not add any test. Would it be nice to have unit test, should i add some?

We feel that exporter-specific functionality should be tested upstream, but any tests of course are welcome. For example, if there's any complicated logic around marshalling/unmarshalling configuration, then a test wouldn't be a bad idea.

Q: Should I create a single PR or split it into multiple one

Like Jan-Otto said, I believe a single PR will suffice.

tpaschalis commented 6 months ago

While the exporter produce 8 metrics for the open-source variant, 129 metrics are available for nginx+ (commercial product) only.

I didn't know, but damn that's sad.

Grafana Cloud Integrations still uses static mode, so we might need this (at least I would need it).

To be fair, in Grafana Cloud's nginx connections, we also suggest that people use the Agent to scrape its JSON logs.

If you plan to use this with Grafana Cloud's static mode, then it might be worth exploring one of the alternatives for a richer set of metrics. In that case, you'd have to wire your static mode code into integrations/v2.

The v1 of static mode exporters (top-level under pkg/integrations) only allows for metrics-based exporters while v2 allows for other types of telemetry as well.

CSalih commented 6 months ago

Thank you both for helping me out.

While the exporter produce 8 metrics for the open-source variant, 129 metrics are available for nginx+ (commercial product) only.

Did not realized that either, this is really bad. I agree with @jkroepke, it does not worth it.

To be fair, in Grafana Cloud's nginx connections, we also suggest that people use the Agent to scrape its JSON logs.

This is sufficient to obverse incoming "requests" but observing the Nginx process itself is not possible (at least with knowledge). Having some metrics like up time, connection, worker usage, resource usage is crucial for me.

If you plan to use this with Grafana Cloud's static mode, then it might be worth exploring one of the alternatives for a richer set of metrics.

I would like to stop the integration of nginxinc/nginx-prometheus-exporter for now. Maybe someone will provide be a exporter with more metrics in future.

github-actions[bot] commented 5 months ago

This issue has not had any activity in the past 30 days, so the needs-attention label has been added to it. If the opened issue is a bug, check to see if a newer release fixed your issue. If it is no longer relevant, please feel free to close this issue. The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your issue will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity. Thank you for your contributions!