feat(inputs.prometheus): Add internal metrics #14424

tguenneguez · 2023-12-08T19:16:11Z

Summary

Be able to have information about collect data off this plugin.

Checklist

[ x] No AI generated code was used in this PR

Related issues

resolves #13103

…mp to readme

plugins/inputs/prometheus/prometheus.go

Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

Hipska

However, I would still only add this metric when enabled in the config.

tguenneguez · 2024-02-05T08:21:07Z

However, I would still only add this metric when enabled in the config.

Hello

544 / 5 000
Résultats de traduction
Résultat de traduction
Hello
It's possible, but what is the point of making the provisioning of these metrics conditional?
Personally, I see 2 advantages in using these conditional options:

A rupture of change
A collection that requires a very significant effort from the plugin.
In this case, I admit that I do not see the point of proposing conditional activation...

If you still want conditional activation, what configuration variable? => enable_request_metrics
Should it be enabled by default?

THANKS
Thomas

Hipska · 2024-02-05T09:31:31Z

No it should not be enabled by default, to keep backwards compatibility. This way users can control whether to have additional metrics.

srebhan

Thanks @tguenneguez! Some small comments...

plugins/inputs/prometheus/prometheus.go

plugins/inputs/prometheus/prometheus_test.go

Good idea Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

srebhan

Thanks @tguenneguez!

powersj · 2024-02-07T16:31:36Z

plugins/inputs/prometheus/prometheus.go

+		}
+	}
+
+	return nil


Are there any other types of errors? What happens if there are?

powersj · 2024-02-07T16:31:39Z

plugins/inputs/prometheus/prometheus.go

+	}
+
+	tags["result"] = resultString
+	requestFields["result_code"] = resultCodes[resultString]


Translating the error string to a numeric value is up to the user and this should not be hard-coded.

The result string can continue to exist as a tag, but the result code field should be dropped.

I understand but I use the same method as here:
https://github.com/influxdata/telegraf/blob/master/plugins/inputs/http_response/http_response.go#L141

powersj · 2024-02-07T16:33:11Z

plugins/inputs/prometheus/prometheus.go

+	var metrics []telegraf.Metric
+	requestFields := make(map[string]interface{})
+	tags := map[string]string{}
+	u.OriginalURL.User = nil


What is the purpose of this?

@tguenneguez thanks for the updates - just another ping on this question.

thanks for the updates - just another ping on this question.

thanks for the updates - just another ping on this question.

I think you meant to respond not just copy my response? :)

Sorry, for what line exactly have you a question ?

u.OriginalURL.User = nil

Ok, I was searching on line :

var metrics []telegraf.Metric
You have a keen eye ;-)
New commit

powersj · 2024-02-07T16:34:57Z

plugins/inputs/prometheus/prometheus.go

@@ -422,6 +491,7 @@ func (p *Prometheus) gatherURL(u URLAndAddress, acc telegraf.Accumulator) error

 	var err error
 	var resp *http.Response
+	start := time.Now()


This should be right before the client.Do and the time captured immediately after the client.Do. That way we avoid the if statements + error checking getting included in the total time.

powersj · 2024-02-07T16:43:34Z

plugins/inputs/prometheus/prometheus.go

-		return fmt.Errorf("error making HTTP request to %q: %w", u.URL, err)
+		if setError(err, requestFields, tags) == nil {
+			// Any error not recognized by `set_error` is considered a "connection_failed"
+			setResult("connection_failed", requestFields, tags)


Capturing the return code and response time of the request is acceptable for a new metric, however, adding all these additional error conditions is not something to start adding to all the plugins. These errors are reported via the the internal metrics already and you can you that or the lack of data as alerts.

powersj · 2024-02-07T17:03:51Z

@tguenneguez,

We agreed to adding the basic support for capturing timing information. Duplicating the entire http_response plugin inside the prometheus plugin is not acceptable.

See this comment and Sven's response: #13103 (comment)

tguenneguez · 2024-02-07T17:56:07Z

I understand your point of view, but when on a telegraf agent we have several prometheus inputs, if we base ourselves on the internal metrics, then it is difficult to remotely identify which source is defective.
This therefore makes it more complicated to deal with the anomaly and especially to solicit the right expert in relation to the defective data source.
As writing in : #13103 (comment)

@tguenneguez after discussing this within the team, we decided to accept a PR adding the proposed metrics to inputs.prometheus. Looking forward to your PR!

The metrics was :
response_time (float, seconds)
content_length (int, response body length)
response_status_code_match (int, 0 = mismatch, 1 = match)
http_response_code (int, response status code)
result_code (int, see below)

…neguez/telegraf into Add-collection-information

powersj · 2024-02-07T19:07:54Z

The metrics was :

No it was not. We should have been more explicitly, but it was in reference to what hipska had proposed previously: response_time and content_length.

then it is difficult to remotely identify which source is defective.

You have also been insistent that you must collect timing information. Which means you are trying to resolve two things in one PR. Do one at a time.

As-is this is not going to land.

telegraf-tiger · 2024-02-12T19:15:58Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

powersj

Thanks for working with us to get this in a great state.

tguenneguez added 13 commits April 27, 2023 20:22

Add oldestFileTimestamp and newestFileTimestamp

7dad9cd

feat(inputs.filecount): Add oldestFileTimestamp and newestFileTimesta…

d4e2720

…mp to readme

Add FileTimestamp option

5664079

Complete sample

55f601e

Add new fields

4fbc02c

Add test new fields

3b7ee53

Remote FileTimestamp flag

75cd4a8

Change default values

f0f196c

Corrige syntaxe

2067812

Modify default values

56484f8

Correct test values

de49a1d

Change test oldest_file_timestamp

41f27a5

Add internal metrics

0b95a6e

tguenneguez changed the title ~~Add internal metrics~~ feat(inputs.prometheus): Add internal metrics Dec 8, 2023

tguenneguez closed this Dec 8, 2023

tguenneguez added 5 commits December 8, 2023 20:22

Correction doc

c84beb0

Correction de la documentation

19838fe

Correction listes

5c83ea4

Correction des tag de list

c5fa0ff

Correction des listes

0e68b07

tguenneguez reopened this Dec 11, 2023

tguenneguez added 3 commits December 11, 2023 11:42

Corrige erreur lint

2521975

Correction champ internalFields

9260484

Corrections variable

2b24a61

Hipska approved these changes Dec 11, 2023

View reviewed changes

plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved

plugins/inputs/prometheus/prometheus.go Outdated Show resolved Hide resolved

tguenneguez and others added 5 commits December 11, 2023 17:07

Update plugins/inputs/prometheus/prometheus.go

83a65ce

Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

Update plugins/inputs/prometheus/prometheus.go

b3aec66

Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>

Ajout de l'option # internal_metric = false

d3a92c9

Découpage lignes

bf61bb1

Correction lignes trop longue

a967e1a

tguenneguez added 2 commits February 2, 2024 17:34

Correction syntaxe

d28e5a3

Add error read data

b431276

Hipska approved these changes Feb 3, 2024

View reviewed changes

Add option enable_request_metrics

ba2a1f1

srebhan reviewed Feb 5, 2024

View reviewed changes

tguenneguez and others added 4 commits February 6, 2024 17:02

Update plugins/inputs/prometheus/prometheus.go

21c82a7

Good idea Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/inputs/prometheus/prometheus_test.go

f02f926

Good idea Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/inputs/prometheus/prometheus_test.go

b2bc12d

Good idea Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/inputs/prometheus/prometheus_test.go

30204db

Good idea Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

srebhan approved these changes Feb 6, 2024

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 6, 2024

srebhan assigned powersj and unassigned srebhan Feb 6, 2024

Hipska approved these changes Feb 7, 2024

View reviewed changes

powersj requested changes Feb 7, 2024

View reviewed changes

tguenneguez added 2 commits February 7, 2024 19:58

Change result as field

b0e18d3

Merge branch 'Add-collection-information' of https://github.com/tguen…

b18c2dc

…neguez/telegraf into Add-collection-information

tguenneguez added 4 commits February 9, 2024 16:21

Compliance with precos

1bf4725

Correction syntaxe

120e878

Correction lint

8337ddc

Line not at the good time

ec969b7

powersj approved these changes Feb 12, 2024

View reviewed changes

powersj merged commit de66a2f into influxdata:master Feb 12, 2024
26 checks passed

github-actions bot added this to the v1.30.0 milestone Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.prometheus): Add internal metrics #14424

feat(inputs.prometheus): Add internal metrics #14424

tguenneguez commented Dec 8, 2023

Hipska left a comment

tguenneguez commented Feb 5, 2024

Hipska commented Feb 5, 2024

srebhan left a comment

srebhan left a comment

powersj Feb 7, 2024

powersj Feb 7, 2024

tguenneguez Feb 7, 2024

powersj Feb 7, 2024

powersj Feb 12, 2024

tguenneguez Feb 12, 2024

powersj Feb 12, 2024

tguenneguez Feb 12, 2024

powersj Feb 12, 2024

tguenneguez Feb 12, 2024

powersj Feb 7, 2024

powersj Feb 7, 2024

powersj commented Feb 7, 2024

tguenneguez commented Feb 7, 2024 •

edited

powersj commented Feb 7, 2024

telegraf-tiger bot commented Feb 12, 2024

Artifact URLs

powersj left a comment

feat(inputs.prometheus): Add internal metrics #14424

feat(inputs.prometheus): Add internal metrics #14424

Conversation

tguenneguez commented Dec 8, 2023

Summary

Checklist

Related issues

Hipska left a comment

Choose a reason for hiding this comment

tguenneguez commented Feb 5, 2024

Hipska commented Feb 5, 2024

srebhan left a comment

Choose a reason for hiding this comment

srebhan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powersj commented Feb 7, 2024

tguenneguez commented Feb 7, 2024 • edited

powersj commented Feb 7, 2024

telegraf-tiger bot commented Feb 12, 2024

Artifact URLs

powersj left a comment

Choose a reason for hiding this comment

tguenneguez commented Feb 7, 2024 •

edited