jaegertracing / helm-charts

Helm Charts for Jaeger backend
Apache License 2.0
267 stars 347 forks source link

[Feature]: add support for "Service Performance Monitoring (SPM)" #440

Open maozza opened 1 year ago

maozza commented 1 year ago

Requirement

I want to enable Service Performance Monitoring (SPM) feature with helm installation install procedure:

helm repo add jaegertracing https://jaegertracing.github.io/helm-charts
helm upgrade --install jaeger jaegertracing/jaeger  --values custom-values.yaml

Problem

will it be possible to add support for Service Performance Monitoring (SPM) in the helm chart? more info can be found here -> https://www.jaegertracing.io/docs/1.41/spm/

Proposal

No response

Open questions

No response

maozza commented 1 year ago

I was trying to add the following values, and I can see that Prometheus scrapes the metrics from the controller and query, but the monitor is still not showing any metrics.

query:
  extraEnv:
  - name: METRICS_STORAGE_TYPE
    value: prometheus
  - name: PROMETHEUS_SERVER_URL
    value: prometheus-kube-prometheus-prometheus.monitoring:9090
  serviceMonitor:
    enabled: true
    additionalLabels:
     release: prometheus
collector:
  serviceMonitor:
    enabled: true
    additionalLabels:
     release: prometheus
jendrikjoe commented 1 year ago

Hi @maozza ,

you will need to deploy the opentelemetry-collector helm chart to achieve this πŸ™‚ The collector takes the spans and produces prometheus metrics from it, if you add the spanmetrics to its pipelinesπŸ™‚ In my specific deployment I could even replace the jaeger collector completely with the open telemetry.

I would be happy to add a respective PR, however a question to the maintainers before doing so: Is this generally a direction you want to go into? πŸ™‚ I can see that an argument can be made to just disable the collector in this chart and deploy the open telemetry one with a separate one. The questions I feel is if you see the case of using the open telemetry collector as the standard one in the future or the jaeger one. Just wanted to figure before I start coding πŸ˜‰

mehta-ankit commented 1 year ago

As I understand it, https://opentelemetry.io/docs/collector/ OTEL collector does not replace Jaeger as a storage/querying piece of tracing data. It receives/processes/exports tracing data.

OTEL collector would still need a backend to send traces to and that could be Jaeger (jaeger can act as an exporter for OTEL collector). So i don't see how you would disable jaeger collector and make the collection of traces and storage of them work end to end.

EDIT: With that being said, Jaeger helm chart should be all about running Jaeger and not OTEL collector. We already have support on this helm chart to receive OTLP format data: https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/templates/collector-deploy.yaml#LL69C19-L69C41

jendrikjoe commented 1 year ago

Thanks for your input @mehta-ankit πŸ™‚ No, it indeed does not replace storage and querying. However, it can use one of its exporters to write directly to the storage backend, e.g. elastic search, from which query could retrieve the traces then πŸ™‚ Right now my setup to enable SPM is as well jaeger-agent -> open telemetry collector -> jaeger-collector -> elastic search πŸ™‚ So if supporting SPM out of the box, I would suggest a similar setup for this chart as it alters it the least πŸ™‚ I just wanted to confirm that the collector to collector approach would be the right one to take for a PR πŸ™‚

FieteO commented 1 year ago

I would appreciate adding this as well. I investigated how to set this up and found the Getting Started instructions in the Jaeger SPM docs much too terse and just boiling down to "just use the all-in-one". I then searched in this chart since this is what I am using to deploy it, but couldn't find anything useful.

ezradibiase commented 7 months ago

@FieteO Have you managed to complete this setup? Because I'm in this very same situation right now.

FieteO commented 7 months ago

@ezradibiase Unfortunately not. I did not pursue it further though.