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

Replace OpenTelemetry with armon/go-metrics #1395

Open
findkim opened this issue Jul 21, 2020 · 15 comments · May be fixed by #1616
Open

Replace OpenTelemetry with armon/go-metrics #1395

findkim opened this issue Jul 21, 2020 · 15 comments · May be fixed by #1616

Comments

@findkim
Copy link
Contributor

findkim commented Jul 21, 2020

Summary

After a few months of weighing the current state of OpenTel vs the current need for metrics wrt to the Consul ecosystem, it has become more evident to me that switching to armon/go-metrics would provide the most cohesive and comprehensive feature set at this time.

Background

PR #1378 recently introduced metrics to Consul Template using OpenTelemetry. There's a short blurb providing context as to why OpenTel was initially chosen over armon/go-metrics and that it was an experiment towards a new library for telemetry.

Context

I tried to create a uniform UX for configuring and emitting metrics over on Consul ESM #70, another Consul ecosystem project, and quickly ran into some short comings when dealing with more complex metric types than counters and histograms. For example, timer values were reduced to histogram integers, and there currently is not a systematic way to denote measurement (seconds, milliseconds, etc).

The community request for statsd is not quite satisfied yet due to the difference in tag syntax between statsd variants and would require omitting the use of tags as a whole to continue support for statsd.

UX

The metrics reported should not change much from the initial work in #1378 (not yet released). However, the telemetry configuration will be breaking changes compared to the PR and align closer to how it is supported by Consul (docs).

Switching libraries will add feature support for:

  • statsd, circonus
  • more metric types, like timers and gauges
  • tags and global tags
  • metrics prefix
  • prefix filtering
@findkim findkim added this to the 0.26.0 milestone Jul 21, 2020
@findkim findkim self-assigned this Jul 21, 2020
@eikenb
Copy link
Contributor

eikenb commented Jul 22, 2020

Would it be better to revert #1378 and start at this fresh or convert the opentel code that is already there?

@findkim
Copy link
Contributor Author

findkim commented Jul 23, 2020

@eikenb good question, I think reverting would be most clear for tracing this pivot when looking back at the history. The original PR could still be used as a reference when making the new changes.

@eikenb
Copy link
Contributor

eikenb commented Jul 23, 2020

Ok, sounds good. I'll get it reverted. Be aware that, because we'll be reverting a merge, if you continue the work off the reverted code you'll need to revert the revert before merging the new code. See this doc for the details... https://github.com/git/git/blob/master/Documentation/howto/revert-a-faulty-merge.txt

@eikenb
Copy link
Contributor

eikenb commented Jul 23, 2020

Opentelemetry merge reverted: 6bc149b

@Oloremo
Copy link

Oloremo commented Nov 22, 2020

Any updates?

@eikenb
Copy link
Contributor

eikenb commented Nov 23, 2020

Hi @Oloremo, no specifics yet. We got sidetracked launching a new tool which ate all our time for a while. If you'd like this to get priority, please 👍 the original issue. We use those to help gauge community priority and how they figure into our prioritized backlog. Thanks.

@Oloremo
Copy link

Oloremo commented Nov 23, 2020

Yeah currently consul-template in a weird state, hashicorp propose it as a critical part of the Nomad infra for example: https://learn.hashicorp.com/tutorials/nomad/vault-pki-nomad yet it's not very clear how to monitor it.

@eikenb
Copy link
Contributor

eikenb commented Nov 23, 2020

Opps. Didn't click through on that vault-pki-nomad link and mistook it for a different case. So ignore the text below (leaving for context). But please be assured we are working on these things. And thanks for adding the 👍.

--

Yeah. It is kind of, but we are working on fixing it with the refactoring of the library case out of consul-template. Once done (I'm actively working on this) it should be a big help as it cleans things up immensely for those use cases (the 'using consul-template as a library' cases). You can follow along at https://github.com/hashicorp/hcat if your interested.

@Oloremo
Copy link

Oloremo commented Nov 23, 2020

Interesting!

Does that mean that the consul-template will be sunsetted soon?

@eikenb
Copy link
Contributor

eikenb commented Nov 23, 2020

As a library, yes. As a stand alone application, no. We'll be updating it to use the new library, but it won't be going anywhere as an application at this point. We're considering merging it and envconsul which, once merged, might end up under a new name, but we're not 100% on that yet.

@Oloremo
Copy link

Oloremo commented Nov 23, 2020

Thanks for the clarification!

@eikenb eikenb removed this from the 0.26.0 milestone May 19, 2021
@Oloremo
Copy link

Oloremo commented Jun 11, 2021

So it missed the 0.26.0...

Will be in 0.27?..

@eikenb
Copy link
Contributor

eikenb commented Jun 11, 2021

Hey @Oloremo,

Sorry that this didn't make it into 0.26.0, I'm working on getting caught up and only wanted to focus on existing PRs and fixing bugs at this point. After this release I need to make a pass of the other projects I maintain (envconsul, consul-esm) and then I'll be putting together some roadmaps for the projects and will determine the priority of this at that time.

If you (and readers) want this, please consider adding a 👍 the top/original issue as I'll look at those when working on the priorities for the roadmap. The roadmaps will also be public and feedback will be welcome there as well.

Thanks.

@Eclion
Copy link

Eclion commented Aug 5, 2022

Hello @eikenb !

I am currently looking into implementing metrics in Consul-Template using armon/go-metrics, as requested in the issue, however I am a bit stuck regarding the implementation of histogram metrics:
The armon/go-metrics library doesn't implement yet histograms and the PR implementing them is open since 2019.
Seeing @findkim did implement histogram metrics in the past, I was planning to implement them too but I am unsure it is possible to easily implement metrics similar to histograms with the available metric types (gauge, counter, summary).

Do you have any opinion regarding the next steps for this issue? Whether it being to ping the maintainer of the aforementioned PR (which I'll do) or to switch to another metric library ?

@Oloremo
Copy link

Oloremo commented Aug 5, 2022

Imo, if the feature is missing it makes sense to implement everything but those metrics and update the metrics after the needed feature will be provided.

Something is better than nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants