Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Prometheus metrics reader factory and config #3049

Merged

Conversation

albertteoh
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Add factories and config for Prometheus metrics reader.

Signed-off-by: albertteoh <albert.teoh@logz.io>
@albertteoh albertteoh requested a review from a team as a code owner June 3, 2021 04:38
@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #3049 (7df9fe7) into master (c0fb781) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3049      +/-   ##
==========================================
+ Coverage   95.98%   96.03%   +0.04%     
==========================================
  Files         225      229       +4     
  Lines        9891     9937      +46     
==========================================
+ Hits         9494     9543      +49     
+ Misses        327      325       -2     
+ Partials       70       69       -1     
Impacted Files Coverage Δ
plugin/metrics/factory.go 100.00% <100.00%> (ø)
plugin/metrics/factory_config.go 100.00% <100.00%> (ø)
plugin/metrics/prometheus/factory.go 100.00% <100.00%> (ø)
plugin/metrics/prometheus/options.go 100.00% <100.00%> (ø)
cmd/collector/app/server/zipkin.go 73.07% <0.00%> (-3.85%) ⬇️
cmd/query/app/server.go 95.62% <0.00%> (-1.46%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0fb781...7df9fe7. Read the comment docs.

jpkrohling
jpkrohling previously approved these changes Jun 3, 2021

import "time"

// Configuration describes the options to customize the storage behavior
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Configuration describes the options to customize the storage behavior
// Configuration describes the options to customize the storage behavior.

plugin/metrics/factory.go Show resolved Hide resolved
case prometheusStorageType:
return prometheus.NewFactory(), nil
}
return nil, fmt.Errorf("unknown metrics type %s. Valid types are %v", factoryType, AllStorageTypes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("unknown metrics type %s. Valid types are %v", factoryType, AllStorageTypes)
return nil, fmt.Errorf("unknown metrics type %q. Valid types are %v", factoryType, AllStorageTypes)

func (f *Factory) CreateMetricsReader() (metricsstore.Reader, error) {
factory, ok := f.factories[f.MetricsStorageType]
if !ok {
return nil, fmt.Errorf("no %s backend registered for metrics store", f.MetricsStorageType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("no %s backend registered for metrics store", f.MetricsStorageType)
return nil, fmt.Errorf("no %q backend registered for metrics store", f.MetricsStorageType)

plugin/metrics/factory_test.go Show resolved Hide resolved
)

const (
suffixHostPort = ".host-port"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, tell me that the next change is about adding TLS support to the communication with Prometheus :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, thanks :) I've added it to the checklist.

Signed-off-by: albertteoh <albert.teoh@logz.io>
@albertteoh albertteoh merged commit 8b1fbe6 into jaegertracing:master Jun 3, 2021
@albertteoh albertteoh deleted the 2954-add-factories-config branch June 3, 2021 10:58
@albertteoh
Copy link
Contributor Author

Thanks @jpkrohling!

@jpkrohling jpkrohling added this to the Release 1.23.0 milestone Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants