PROMETHEUS_SERVER_URL broken since 1.34

[greut]

Describe the bug
After upgrading to 1.34 from 1.33 the PROMETHEUS_SERVER_URL value is not read anymore.

Using v1.33

{"level":"info","ts":1652451118.0158124,"caller":"metricsstore/reader.go:94",
 "msg":"Prometheus reader initialized","addr":"http://172.16.128.175:9090"} 

And v1.34, which shows the default value.

{"level":"info","ts":1652451438.1780195,"caller":"metricsstore/reader.go:94",
 "msg":"Prometheus reader initialized","addr":"http://localhost:9090"}

To Reproduce
Steps to reproduce the behavior:

1. Run 1.33 with monitor tab
2. Upgrade to 1.34
3. Notice the 500's error in the monitor tab

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Version (please complete the following information):

• OS: Linux
• Jaeger version: 1.34
• Deployment: Docker

What troubleshooting steps did you try?

I was using PROMETHEUS_SERVERURL env variable and switched to a config-file, same issue. Here is the config file.

log-level: info

admin:
  http:
    host-port: "0.0.0.0:29937"

query:
  base-path: "/jaeger"
  grpc-server:
    host-port: "0.0.0.0:24507"
  http-server:
    host-port: "0.0.0.0:25050"

es:
  server-urls: "http://opensearch-tracing.service.consul:9200"

prometheus:
  server-url: "http://172.16.128.175:9090"

Additional context

Asked for help on Slack: https://cloud-native.slack.com/archives/CGG7NFUJ3/p1652443407988039

[albertteoh]

Thanks for reporting the bug @greut.

This bug was introduced in the PR: #3030

Specifically, the signature of InitFromViper was changed to include an error return: https://github.com/jaegertracing/jaeger/pull/3030/files#diff-cda5a3c6a9402682254064b89ae99ab89f5b761568f309ccf37664bdf3516a8cR72

This meant that the prometheus factory no longer implemented the plugin.Configurable interface and so its options, such as server-url, would not be registered.

Fix options:

1. Remove the error return so that it implements plugin.Configurable and instead, log.Fatal on error.
2. Change the InitFromViper interface func to return error and refactor all implementations.

Option 1. involves the least changes, arguably okay to crash on startup due to invalid arguments, and would be consistent with behaviour from other storage types:

• Cassandra
• ES
• Kafka

IMO option 2. is the more correct approach, but involves more changes.

What do folks think? Happy to put in a fix either way.

[yurishkuro]

I would prefer option 2, but you should see how much changes that would involve.

If changing the signature introduced this bug, then the factory did not have a unit test where it was cast plugin.Configurable, we need to add that.

[robgaoo]

I also found this problem and saw that the log kept requesting localhost9090

[yurishkuro]

@albertteoh on the other hand, I think we should first try to fix the immediate issue with minimal changes (e.g. remove error return and panic inside the factory) and release a patch.

[yurishkuro]

A fix released in https://github.com/jaegertracing/jaeger/releases/tag/v1.34.1

Apologies for the churn.

[greut]

@yurishkuro thanks, deployed and working fine! Much appreciated.