Skip to content
This repository has been archived by the owner on Mar 8, 2022. It is now read-only.

Telegraf/Influx converter #1

Merged
merged 5 commits into from
Mar 24, 2021
Merged

Telegraf/Influx converter #1

merged 5 commits into from
Mar 24, 2021

Conversation

FZambia
Copy link
Contributor

@FZambia FZambia commented Mar 23, 2021

Added converter from telegraf/influx format to Grafana Data Frames. This converter generates one frame for each input metric name and time combination.

Not sure about naming - maybe it's better to call package influx instead of telegraf (since input is in influx format, but it contains metrics which are telegraf.Metric).

Benchmark:

BenchmarkConverter_Convert-12    	 13599	   85522 ns/op	 74312 B/op	  1360 allocs/op

@FZambia FZambia requested a review from ryantxu March 23, 2021 15:25
}

// FrameWrapper is a wrapper over data.Frame.
type FrameWrapper interface {
Copy link
Member

Choose a reason for hiding this comment

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

fine for now... but maybe we should add "key" as a standard frame property 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, maybe not - because the key here used mostly to construct a channel name to publish data into, maybe better try to keep live specifics separate from general data frame logic as long as possible?

frameWrappers, err := converter.Convert(testData)
require.NoError(t, err)
require.Len(t, frameWrappers, 1)

Copy link
Member

Choose a reason for hiding this comment

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

consider adding a "golden" test -- I find these super userful for understanding input vs output -- and would love your help/advice making the core interface better :)

See:
https://github.com/grafana/grafana/blob/master/pkg/tsdb/influxdb/flux/executor_test.go#L70
Or:
https://github.com/grafana/oas-datasource/blob/fa755294643e56371afe326850aaf76eb0bb6f88/pkg/server/test/server_test.go#L69

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)

// FieldTypeFor returns a concrete type for a given interface or unknown if not known
func FieldTypeFor(t interface{}) data.FieldType {
Copy link
Member

Choose a reason for hiding this comment

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

eventually in the SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

Looks good -- can you try the golden file test. I find it a super helpful pattern

@FZambia FZambia merged commit 0db61a9 into main Mar 24, 2021
@FZambia FZambia deleted the FZambia/telegraf_converter branch March 24, 2021 09:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants