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

Use peer id as metric label #409

Merged
merged 2 commits into from
Sep 15, 2020
Merged

Conversation

coignetp
Copy link
Contributor

@coignetp coignetp commented Jul 8, 2020

Hey!
I'm currently working on the consul integration at Datadog, and we want to scrape the prometheus endpoint to get consul metrics. In the current state several metrics are not consistently named, and contain parts that would be good as labels instead, and that makes scraping the endpoint and converting to Datadog metrics troublesome.
This PR use the peer ids as label and remove them from the metric names.

Example: consul_raft_replication_appendEntries_logs_684ad51c_6143_f2ea_aaaa_8ab6cc5ccccc -> consul_raft_replication_appendEntries_logs.
This PR affects:

  • consul_raft_replication_installSnapshot
  • consul_raft_replication_heartbeat
  • consul_raft_replication_appendEntries_rpc
  • consul_raft_replication_appendEntries_logs

This change is not backward compatible since the metric names will be different. The current metric parsing method won't work anymore.

Please tell me what you think about this change! 😄

P.S. : I'd like to do the same thing for other consul metrics (here for example https://github.com/hashicorp/consul/blob/5e5dbedd47a5f875b60e241c5555a9caab595246/agent/http.go#L258)

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 8, 2020

CLA assistant check
All committers have signed the CLA.

@coignetp
Copy link
Contributor Author

coignetp commented Jul 9, 2020

Is the CI failing because of my change? I'm not sure I understand the error here

@schristoff
Copy link
Contributor

Howdy - this is great! Thank you so much.
Is there any way we could maintain the original metrics and add the new metrics as well to maintain backwards compatibility?

@coignetp
Copy link
Contributor Author

I updated the PR to keep sending both metrics

@coignetp
Copy link
Contributor Author

coignetp commented Jul 20, 2020

@s-christoff Should I do something more on this change? I'm not sure if the CI is failing because of this PR here🙂

@schristoff
Copy link
Contributor

schristoff commented Jul 24, 2020

Howdy friend,
Thank you for your patience and hard work on this. It's started some internal discussions - which is great! We're (Consul) looking into deprecating other metadata-suffixed metrics in Consul and we are working through the details. We're going to figure some stuff out on our end, and will be looking to accept this soon.

@coignetp
Copy link
Contributor Author

Hi @s-christoff, is there any news about this?

@coignetp
Copy link
Contributor Author

Hi @s-christoff, sorry to bother you again, but how do you have any news here?

Copy link
Contributor

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

lgtm - sorry for the delay, lots of internal conversations happening. :p

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.

3 participants