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

Support prometheus #168

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support prometheus #168

wants to merge 3 commits into from

Conversation

mmpei
Copy link
Contributor

@mmpei mmpei commented May 30, 2019

#161
add 'http_' as prefix of metrics name. for there is a restriction in prometheus that metrics name should start with letter.
and you can set backend to 'promethues' to active prometheus metrics.

metrics:
  backend: prometheus
  m3:
    service: kraken-agent
    host_port: 10740
  statsd:
    host_port: 0.0.0.0:10740
    prefix: kraken-agent
  prometheus:
    listen_address: 0.0.0.0:10740

Copy link
Contributor

@codygibb codygibb left a comment

Choose a reason for hiding this comment

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

This is going to break all of our existing monitoring. Is there a way to make this backwards compatible?

@evelynl94
Copy link
Collaborator

This is going to break all of our existing monitoring. Is there a way to make this backwards compatible?

+1, maybe its better to wrap promethues metrics to add a prefix.

@evelynl94
Copy link
Collaborator

also http_ usually implies its related to http endpoints...you should just use kraken_

@mmpei
Copy link
Contributor Author

mmpei commented May 31, 2019

This is going to break all of our existing monitoring. Is there a way to make this backwards compatible?

+1, maybe its better to wrap promethues metrics to add a prefix.

it is difficult to do. i have considered that but it will break the abstract architecture if only add prefix to promethues. i will try.

@mmpei
Copy link
Contributor Author

mmpei commented May 31, 2019

also http_ usually implies its related to http endpoints...you should just use kraken_

you are right. maybe kraken_http_ to identify that it's from kraken and related to http.

@yiranwang52
Copy link
Collaborator

I actually think this change is fine...let's discuss a little more about this

type PrometheusConfig struct {
ListenAddress string `yaml:"listen_address"`
// HandlerPath if not define use default /metrics.
HandlerPath string `yaml:"handler_path"`
Copy link

Choose a reason for hiding this comment

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

The yaml config keys are camel cased in the tally package:

https://github.com/uber-go/tally/blob/f266f90e9c4d5894364039a324a05d061f2f34e2/prometheus/config.go#L33-L58

Perhaps we should prefer camel case here as well so that it's not a breaking change if we want to swap PrometheusConfig out with the tally prometheus.Configuration type in the future.

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.

5 participants