fix(outputs.stackdriver): Options to use official path and types #13454

powersj · 2023-06-15T16:46:18Z

This reverts commit 275c706.

This reverts commit 6f59957.

lyanco · 2023-06-21T20:51:40Z

plugins/outputs/stackdriver/stackdriver.go

+
+	var kind string
+	switch m.Type() {
+	case telegraf.Gauge, telegraf.Untyped:


This line is saying that when a metric is untyped to send it in as a gauge. From our experience with tons of customers using GMP, this logic will definitely lead to incorrect behavior. Tons of metrics get untyped at the source for reasons unknown to me, and we have to account for this because people really don't want to change their exporters.

The way we handle this with our other collectors is, when a metric is untyped, to send it twice: once with the suffix "unknown" and once with the suffix "unknown:counter". Then we do some heuristic magic on the query side to choose the gauge (unknown) or the counter (unknown:counter) depending on our best guess based on the query functions you use. If you then type the metric, we union the data.

This is a bit larger of a change but without it this commit will 100% lead to unhappy customers on our (Google's) end.

Thanks for taking a look at the PR and giving insight into how this ends up getting used.

The way we handle this with our other collectors is, when a metric is untyped, to send it twice

Because the two points would have different metric types, both can be sent as part of the same timeseries, the only difference being the suffix of the metric type?

This is a bit larger of a change but without it this commit will 100% lead to unhappy customers on our (Google's) end.

fwiw this change will not be the default behavior and is opt-in. We do not want to change things for existing users, who as of now are not even setting these types.

Tons of metrics get untyped at the source for reasons unknown to me

My understanding is any metric from Telegraf not read from a Prometheus or using the Prometheus parser is internally stored as an untyped metric.

Thanks again!

Thanks for taking a look at the PR and giving insight into how this ends up getting used.

The way we handle this with our other collectors is, when a metric is untyped, to send it twice

Because the two points would have different metric types, both can be sent as part of the same timeseries, the only difference being the suffix of the metric type?

Yeah - you can send the two metrics in the same call for sure. They'd basically be identical time series with just the suffix of the metric name changed. In our system they're treated as two separate metrics (as they have different metric names due to the suffix), so there are no collisions or anything if you send both together.

This is a bit larger of a change but without it this commit will 100% lead to unhappy customers on our (Google's) end.

fwiw this change will not be the default behavior and is opt-in. We do not want to change things for existing users, who as of now are not even setting these types.

Ack - makes sense, thanks.

Tons of metrics get untyped at the source for reasons unknown to me

My understanding is any metric from Telegraf not read from a Prometheus or using the Prometheus parser is internally stored as an untyped metric.

Yeah - we're running into this issue with a different customer using influxdb. One of our official collectors isn't doing the write-twice thing by accident (bug), and they're unable to query counter metrics properly as a result. There also doesn't seem to be a way for them to change it at the source without modifying the exporter code which they're loathe to do, so they're stuck until we can make an upstream change to our collector.

Totally understandable from them and a very common situation, which is why handling untyped metrics properly at ingestion time is so important on our end.

Thanks again!

Awesome, I have pushed a change to:

Mark untyped metrics with "unknown"

If we are using the official metric name format, and we run across a metric with an unknown kind, then we will create another timeseries, identical, but with the "unknown:counter" kind.

I need to add some additional test cases, but wanted to get this in folks hands quickly to try out.

…nown:counter kind

crflanigan · 2023-06-23T15:14:01Z

I have been running this for about 20 hours and everything seems to be working correctly.
@powersj Have you tested this with histograms?

powersj · 2023-06-23T16:14:47Z

@crflanigan,

I have been running this for about 20 hours and everything seems to be working correctly.

Awesome, thank you for the feedback!

Have you tested this with histograms?

Histograms and summary types are not supported by the plugin and will produce an error. I am sure we could look at changing that in the future with some help.

crflanigan · 2023-06-23T16:56:09Z

@crflanigan,

I have been running this for about 20 hours and everything seems to be working correctly.

Awesome, thank you for the feedback!

Have you tested this with histograms?

Histograms and summary types are not supported by the plugin and will produce an error. I am sure we could look at changing that in the future with some help.

No problem, do we have anything outstanding before we merge? @powersj

powersj · 2023-06-23T21:00:14Z

I need to update the README with a bit more info and get a final review from @srebhan. I'll work on the README now. Thanks again for the feedback!

srebhan

Nice! Just one small typo...

plugins/outputs/stackdriver/stackdriver.go

srebhan

Commited the typo fix... Thanks @powersj!

telegraf-tiger · 2023-06-26T10:37:13Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.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

…luxdata#13454)

) (cherry picked from commit 45f9942)

fix(outputs.stackdriver): Options to use official path and types

7ab1a7b

fixes: influxdata#13303

telegraf-tiger bot added fix pr to fix corresponding bug plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Jun 15, 2023

powersj mentioned this pull request Jun 15, 2023

Improve outputs.stackdriver to better support GMP use case #13303

Closed

powersj self-assigned this Jun 15, 2023

make namespace optional

275c706

influxdata deleted a comment from telegraf-tiger bot Jun 20, 2023

powersj added 2 commits June 20, 2023 14:04

Revert "make namespace optional"

6f59957

This reverts commit 275c706.

Revert "Revert "make namespace optional""

58ad6b9

This reverts commit 6f59957.

lyanco reviewed Jun 21, 2023

View reviewed changes

powersj added 2 commits June 22, 2023 12:04

untyped metric handling: mark unknown, create 2nd timeseries with unk…

e6717fc

…nown:counter kind

update metrickind on uknown metrics

e74668b

use double as value, update docs

016a774

powersj 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 Jun 23, 2023

powersj assigned srebhan and unassigned powersj Jun 23, 2023

powersj marked this pull request as ready for review June 23, 2023 21:06

srebhan reviewed Jun 26, 2023

View reviewed changes

plugins/outputs/stackdriver/stackdriver.go Outdated Show resolved Hide resolved

Update plugins/outputs/stackdriver/stackdriver.go

f6fc51a

srebhan approved these changes Jun 26, 2023

View reviewed changes

srebhan assigned powersj and unassigned srebhan Jun 26, 2023

powersj merged commit 45f9942 into influxdata:master Jun 26, 2023

powersj deleted the fix/13303 branch June 26, 2023 13:30

powersj added a commit to powersj/telegraf that referenced this pull request Jul 5, 2023

fix(outputs.stackdriver): Options to use official path and types (inf…

445a4eb

…luxdata#13454)

powersj added a commit that referenced this pull request Jul 10, 2023

fix(outputs.stackdriver): Options to use official path and types (#13454

1bd9f2d

) (cherry picked from commit 45f9942)

srebhan added this to the v1.27.2 milestone Jul 10, 2023

p-linnane mentioned this pull request Jul 10, 2023

telegraf 1.27.2 Homebrew/homebrew-core#136267

Merged

powersj mentioned this pull request Aug 9, 2023

Outputs.Stackdriver Start and End Timestamp Appear to be the Same #13725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(outputs.stackdriver): Options to use official path and types #13454

fix(outputs.stackdriver): Options to use official path and types #13454

powersj commented Jun 15, 2023

lyanco Jun 21, 2023

powersj Jun 21, 2023

lyanco Jun 22, 2023

powersj Jun 22, 2023

crflanigan commented Jun 23, 2023

powersj commented Jun 23, 2023

crflanigan commented Jun 23, 2023 •

edited

Loading

powersj commented Jun 23, 2023

srebhan left a comment

srebhan left a comment

telegraf-tiger bot commented Jun 26, 2023

Artifact URLs

fix(outputs.stackdriver): Options to use official path and types #13454

fix(outputs.stackdriver): Options to use official path and types #13454

Conversation

powersj commented Jun 15, 2023

lyanco Jun 21, 2023

Choose a reason for hiding this comment

powersj Jun 21, 2023

Choose a reason for hiding this comment

lyanco Jun 22, 2023

Choose a reason for hiding this comment

powersj Jun 22, 2023

Choose a reason for hiding this comment

crflanigan commented Jun 23, 2023

powersj commented Jun 23, 2023

crflanigan commented Jun 23, 2023 • edited Loading

powersj commented Jun 23, 2023

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Jun 26, 2023

Artifact URLs

crflanigan commented Jun 23, 2023 •

edited

Loading