Implements metrics using armon/go-metrics library #1616

Eclion · 2022-08-09T08:57:26Z

As suggested in the issue #1395, this PR implements a metric system using the armon/go-metrics library along with few metrics.

To achieve such implementation, the metric system was based on the implementation done in Consul and the exposed metrics have been implemented based on the PR #1378.

However, this implementation only partially implements the mentioned metrics:

the armon/go-metrics library doesn't support yet histograms so these metrics are not implemented in this PR.
to avoid import cycling between the dependency, config and telemetry packages, metrics related to the vault token were not implemented

Resolves #1395

hashicorp-cla · 2022-08-09T08:57:30Z

All committers have signed the CLA.

Eclion · 2022-08-23T12:53:00Z

Hello @eikenb !
Do you perhaps know someone who may be available to review this PR ?

c0deaddict · 2023-01-26T07:15:48Z

Any update on this? Would love to see this merged. We're kind of in the dark now when consul-template fails to render a template. Except when a service using the rendered template eventually breaks, but I would like to know that before hand.

Oloremo · 2023-04-18T13:13:54Z

Any updates on this?

cybervedaa · 2023-05-18T23:44:34Z

+1 for this please. this will go a long way in reliably monitoring consul-template

roncodingenthusiast

🎉 @Eclion Thank you so much for the thorough work that went into making this PR happen.
Happy to work with you and a few others to get this merged/released within the next couple of weeks.

✍🏾 I did a first pass review and i am leaving a few comments for consideration. Let me know if any of them don't make sense to you.

❓ Do you mind updating this PR to the main branch, looks like it is quite behind now so it will be nice to get this updated before we work on merging it in

🔢 Happy to help/assist in anyway you want me to

README.md

telemetry/metrics.go

config/convert.go

manager/runner.go

telemetry/telemetry.go

Eclion · 2023-06-22T08:18:18Z

tada @Eclion Thank you so much for the thorough work that went into making this PR happen. Happy to work with you and a few others to get this merged/released within the next couple of weeks.

Hello @roncodingenthusiast !
Thank you very much for your PR!

✍🏾 I did a first pass review and i am leaving a few comments for consideration. Let me know if any of them don't make sense to you.

question Do you mind updating this PR to the main branch, looks like it is quite behind now so it will be nice to get this updated before we work on merging it in

Sure! I will rebase my branch before working on your comments.

1234 Happy to help/assist in anyway you want me to

c0deaddict · 2023-09-22T06:44:28Z

I would very much like for this PR to be merged - and released. Any update on this?

cybervedaa · 2023-10-05T03:13:41Z

I'm eagerly waiting on this to be merged as well. Can't wait for this to be merged

Eclion · 2024-04-15T08:54:30Z

fork rebased on current main branch

hkrutzer · 2024-06-19T11:48:55Z

Hoping this year will be the year of consul-template metrics. @nathancoleman @jmurret @jm96441n @DanStough I see you have recently done work on this repo, perhaps one of you can give a status update?

implements the metric system following the implementation done in Consul and trying to implement the same metrics as done in the previous metric system (c.f. PR1378: https://github.com/hashicorp/consul-template/pull/1378/files )

@roncodingenthusiast

following @roncodingenthusiast's suggestion

@roncodingenthusiast

following @roncodingenthusiast's suggestion

jm96441n

overall looks good, just a few additional changes and I'm happy to merge

jm96441n · 2024-10-09T20:16:55Z

config/telemetry.go

+// Merge combines all values in this configuration with the values in the other
+// configuration, with values in the other configuration taking precedence.
+// Maps and slices are merged, most other values are overwritten.
+func (c *TelemetryConfig) Merge(o *TelemetryConfig) *TelemetryConfig {


can we add tests for this merge function?

Sure!
I added some tests as requested

jm96441n · 2024-10-09T20:17:14Z

config/telemetry.go

+	return fmt.Sprintf("&TelemetryConfig{"+
+		"Disable:%v, "+
+		"CirconusAPIApp:%s, "+
+		"CirconusAPIToken:%s, "+


are we worried about leaking the API token in logs here?

Thanks for spotting this issue. The output value has been replaced by <empty> and <configured>.

jm96441n · 2024-10-09T20:18:12Z

dependency/dependency.go

@@ -49,6 +49,14 @@ const (
 	DefaultContextTimeout = 60 * time.Second
 )

+func (t Type) String() string {
+	if t > 2 {


minor: could we have a comment here indicating why 2 or use a named constant?

It was used in the upstream PR to default to unknown probably because Nomad was not part of the Type list at that time.
I replaced this section by a switch case, more explicit

jm96441n · 2024-10-09T20:22:13Z

manager/runner.go

@@ -1478,3 +1511,30 @@ func newWatcher(c *config.Config, clients *dep.ClientSet) *watch.Watcher {
 		RetryFuncNomad:   watch.RetryFunc(c.Nomad.Retry.RetryFunc()),
 	})
 }
+
+func recordDependencyCounts(deps map[string]dep.Dependency) {


this function doesn't return anything or modify the input in any way, what is it meant to do?

This section of code from the PR #1378 was used to compute metrics for the histograms.
However, as histograms are yet to be implemented in go-metrics, this code is therefor unneeded.
I will remove it.

jm96441n · 2024-10-09T20:30:23Z

telemetry/telemetry_test.go

+	for _, actualMetric := range actualMetrics {
+		if strings.HasPrefix(actualMetric, "# HELP go_") ||
+			strings.HasPrefix(actualMetric, "# TYPE go_") ||
+			strings.HasPrefix(actualMetric, "go_") ||
+			strings.HasPrefix(actualMetric, "# HELP process_") ||
+			strings.HasPrefix(actualMetric, "# TYPE process_") ||
+			strings.HasPrefix(actualMetric, "process_") ||
+			actualMetric == "" {
+			continue
+		}
+


minor but this might be a bit more readable

Suggested change

for _, actualMetric := range actualMetrics {

if strings.HasPrefix(actualMetric, "# HELP go_") ||

strings.HasPrefix(actualMetric, "# TYPE go_") ||

strings.HasPrefix(actualMetric, "go_") ||

strings.HasPrefix(actualMetric, "# HELP process_") ||

strings.HasPrefix(actualMetric, "# TYPE process_") ||

strings.HasPrefix(actualMetric, "process_") ||

actualMetric == "" {

continue

}

prefixes := []string{"# HELP go_", "#TYPE go", "go_", "# HELP process_", "# TYPE process_", "process_"}

for _, actualMetric := range actualMetrics {

if slices.ContainsFunc(prefixes, func(p string) bool { return strings.HasPrefix(actualMetric, p) }) ||

actualMetric == "" {

continue

}

Thanks for the suggestion! I agree with it being more readable.

…etricActivation'

…regarding prefix check on array

…tion doc

Eclion · 2024-10-14T12:18:16Z

Hello @jm96441n,
Thank you very much for your review!!
I updated the code to address your comments. Could you please review them once you have a bit of time?

tgross

Nomad embeds consul-template as a library rather than using it as a standalone binary. The HTTP server and telemetry sink should be configurable by passing in an existing server and sink.

…se it's shutdown function

Eclion · 2024-10-14T16:01:29Z

Nomad embeds consul-template as a library rather than using it as a standalone binary. The HTTP server and telemetry sink should be configurable by passing in an existing server and sink.

Hello @tgross ,

If I am not wrong, there is no need to call telemetry.Init() if metrics.NewGlobal is called in the code somewhere.

So the only needed call, as far as I understand, would be telemetry.CounterTemplatesRendered.Add(1) when a template is rendered.

However, such call is not implemented in the renderer.Render method, used by Nomad, but in the manager.RunnerRun. method, where we can also keep track of which template was rendered.

tgross · 2024-10-14T17:43:17Z

However, such call is not implemented in the renderer.Render method, used by Nomad, but in the manager.RunnerRun. method, where we can also keep track of which template was rendered.

The Render call there is only so that Nomad sandbox the rendered. Nomad calls manager.Runner.Start() which in turn calls manager.Runner.Run(). But to your point, you're right only the cli here is calling Init and instantiating the sink. No objections from our end.

Eclion requested a review from a team August 9, 2022 08:57

eikenb added the enhancement label Aug 12, 2022

Eclion force-pushed the armon-gometrics-implementation branch from 7cce0dd to 9fc25e6 Compare August 19, 2022 12:57

eikenb force-pushed the main branch from e04ca78 to 32e41ed Compare March 7, 2023 19:01

roncodingenthusiast self-requested a review June 21, 2023 15:34

roncodingenthusiast reviewed Jun 21, 2023

View reviewed changes

roncodingenthusiast added the waiting-reply label Jun 21, 2023

Eclion force-pushed the armon-gometrics-implementation branch from 997b530 to 82c33c4 Compare June 22, 2023 08:29

Eclion requested a review from a team as a code owner June 22, 2023 08:29

Eclion force-pushed the armon-gometrics-implementation branch from 43166c3 to 80b5f0a Compare June 22, 2023 10:10

Eclion requested a review from roncodingenthusiast July 26, 2023 08:57

Eclion force-pushed the armon-gometrics-implementation branch from 750c214 to 011d417 Compare October 20, 2023 12:04

Eclion requested review from a team and kisunji and removed request for a team October 20, 2023 12:04

kisunji removed their request for review January 5, 2024 21:00

Eclion force-pushed the armon-gometrics-implementation branch from 011d417 to ec189f9 Compare April 15, 2024 08:53

Eclion added 3 commits August 23, 2024 17:28

Corrects the telemetry config comments

6395482

prevents crashes due to empty Telemetry config

2dce549

Eclion added 10 commits August 23, 2024 17:28

fixes README typo

c2060d9

removes unused methods from config.convert

20aae40

provides context for method TelemetryConfig.Merge

b6e1ca4

reformats metrics label computing in runner

fbdfc8f

following @roncodingenthusiast's suggestion

simplifies method CounterMetric.Add

b4445ce

initializes all counters

30d31a3

removes debug logging

36b2819

uses multierror for metrics sink configuration

5cdbb6d

following @roncodingenthusiast's suggestion

removes duplicate metrics sink addition

4742f4b

removes commented metrics call

4caed4f

Eclion force-pushed the armon-gometrics-implementation branch from ec189f9 to 4caed4f Compare August 23, 2024 15:31

Eclion added 2 commits August 26, 2024 11:20

adds armon/go-metrics to allowed depencies for golantci-linter

95f391d

adds promclient dependency to golangci-lint

5e7bb54

jm96441n requested changes Oct 9, 2024

View reviewed changes

Eclion added 9 commits October 11, 2024 10:53

fix(config/telemetry): prevents displaying the circonus API token

61e9218

fix(config/telemetry): uses the correct type for 'CirconusCheckForceM…

552ffaa

…etricActivation'

feat(config/telemetry): adds config merge tests

80287f9

feat(config/telemetry): adds gostring test for telemetry config

7762510

fix(dependency/type): makes use of switch-case for Type's String method

37fc559

fix(manager/runner): removes unneeded recordDependencyCounts method

81d87ce

fix(telemetry_test/formatting): applies @jm96441n 's recommendations …

c7b4679

…regarding prefix check on array

fix(config/telemetry): corrects some circonus config json field

1b4810c

feat(telemetry/doc): adds a telemetry config example in the configura…

6dd0f56

…tion doc

Eclion requested a review from jm96441n October 14, 2024 12:18

tgross reviewed Oct 14, 2024

View reviewed changes

Eclion added 2 commits October 14, 2024 17:34

fix(telemetry/init): extracts the Metrics server init in order to reu…

4ae8d02

…se it's shutdown function

fix(telemetry/test): fixes typo in go metrics prefixes

c8f4f0f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements metrics using armon/go-metrics library #1616

Implements metrics using armon/go-metrics library #1616

Eclion commented Aug 9, 2022

hashicorp-cla commented Aug 9, 2022 •

edited

Loading

Eclion commented Aug 23, 2022

c0deaddict commented Jan 26, 2023

Oloremo commented Apr 18, 2023

cybervedaa commented May 18, 2023

roncodingenthusiast left a comment •

edited

Loading

Eclion commented Jun 22, 2023

c0deaddict commented Sep 22, 2023

cybervedaa commented Oct 5, 2023

Eclion commented Apr 15, 2024

hkrutzer commented Jun 19, 2024

jm96441n left a comment

jm96441n Oct 9, 2024

Eclion Oct 11, 2024

jm96441n Oct 9, 2024

Eclion Oct 11, 2024

jm96441n Oct 9, 2024

Eclion Oct 11, 2024

jm96441n Oct 9, 2024

Eclion Oct 11, 2024

jm96441n Oct 9, 2024

Eclion Oct 11, 2024

Eclion commented Oct 14, 2024

tgross left a comment

Eclion commented Oct 14, 2024

tgross commented Oct 14, 2024

Implements metrics using armon/go-metrics library #1616

Are you sure you want to change the base?

Implements metrics using armon/go-metrics library #1616

Conversation

Eclion commented Aug 9, 2022

hashicorp-cla commented Aug 9, 2022 • edited Loading

Eclion commented Aug 23, 2022

c0deaddict commented Jan 26, 2023

Oloremo commented Apr 18, 2023

cybervedaa commented May 18, 2023

roncodingenthusiast left a comment • edited Loading

Choose a reason for hiding this comment

Eclion commented Jun 22, 2023

c0deaddict commented Sep 22, 2023

cybervedaa commented Oct 5, 2023

Eclion commented Apr 15, 2024

hkrutzer commented Jun 19, 2024

jm96441n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eclion commented Oct 14, 2024

tgross left a comment

Choose a reason for hiding this comment

Eclion commented Oct 14, 2024

tgross commented Oct 14, 2024

hashicorp-cla commented Aug 9, 2022 •

edited

Loading

roncodingenthusiast left a comment •

edited

Loading