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

Adding new Telegraf input for t128_peerpath #55

Conversation

shriyanshk128T
Copy link

@shriyanshk128T shriyanshk128T commented Aug 29, 2023

Description

In order to translate peer_path to native telegraf we decided to create a new input telegraf which would use the existing mechanism of t128_graph and would implement conditions accordingly.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@gregschrock gregschrock changed the title Adding new Telegraf input, T128_event_collector and T128_peerpath Adding new Telegraf input, t128_event_collector and t128_peerpath Aug 30, 2023
plugins/inputs/all/all.go Outdated Show resolved Hide resolved
plugins/inputs/all/all.go Show resolved Hide resolved
@@ -0,0 +1,128 @@
# 128T Peer Path Input Plugin
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a better way to replicate this code. It's a tricky thing because the query is version dependent and we rely on the Monitoring Agent for all version dependent inputs. But it's not valid to configure an arbitrary GraphQL query in here as we will have post-processing logic specific to a query.

I think what we should have is this:

[[intpus.t128_peer_path]]
base_url = "..."
unix_socket = "..."
timeout = "..."

# versioned fields
exclude_hostname = false

And then for the implementation, it would look something like this:

type T128PeerPath struct {
	CollectorName   string            `toml:"collector_name"`
	BaseURL         string            `toml:"base_url"`
	UnixSocket      string            `toml:"unix_socket"`
	Timeout         config.Duration   `toml:"timeout"`
	ExcludeHostname bool 			  `toml:"exclude_hostname"`
	gqlCollector *t128_graphql.T128GraphQL
}

// SampleConfig returns the default configuration of the Input
func (*T128PeerPath) SampleConfig() string {
	return sampleConfig
}

//Init sets up the input to be ready for action
func (plugin *T128PeerPath) Init() error {
	// parse the config
	plugin.gqlCollector.BaseURL = plugin.BaseURL
	plugin.gqlCollector.UnixSocket= plugin.UnixSocket
	// populate the gqlCollector fields etc.

	plugin.gqlCollector.Init()
}

// Gather takes in an accumulator and adds the metrics that the Input gathers
func (plugin *T128PeerPath) Gather(acc telegraf.Accumulator) error {
	metricsChan := make(chan telegraf.Metric)
	redirectAccumulator := agent.NewAccumulator(metricChan, /* other args*/)

	go func() {
		plugin.gqlCollector.Gather(redirectAccumulator)
		close(metricsChan)
	}()

	for  metric := range metricsChan {
		// conditional logic goes here to adjust the metric and decide if it should be included, then:
		acc.AddFields(metric /*args need to adjust*/)
	}
}

@shriyanshk128T shriyanshk128T changed the title Adding new Telegraf input, t128_event_collector and t128_peerpath Adding new Telegraf input for t128_peerpath Aug 30, 2023
Copy link

@BenMatase BenMatase left a comment

Choose a reason for hiding this comment

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

Please use another method. Either figure out a way to create a graphql input internally within the peer path input or refactor the code so that they can both use it.

plugins/inputs/t128_graphql/t128_graphql.go Outdated Show resolved Hide resolved
plugins/inputs/t128_graphql/t128_graphql.go Outdated Show resolved Hide resolved
plugins/inputs/t128_graphql/t128_graphql.go Outdated Show resolved Hide resolved
plugins/inputs/t128_peer_path/t128_peer_path.go Outdated Show resolved Hide resolved
@shriyanshk128T shriyanshk128T marked this pull request as ready for review September 6, 2023 12:30
plugins/inputs/t128_graphql/t128_graphql.go Outdated Show resolved Hide resolved
plugins/inputs/t128_peer_path/t128_peer_path.go Outdated Show resolved Hide resolved
plugins/inputs/t128_peer_path/t128_peer_path.go Outdated Show resolved Hide resolved
plugins/inputs/t128_peer_path/t128_peer_path.go Outdated Show resolved Hide resolved
plugins/inputs/t128_peer_path/t128_peer_path.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gregschrock gregschrock left a comment

Choose a reason for hiding this comment

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

This seems pretty good to me, but it will require some system testing. Let's

  1. Create a feature branch for the various improvements you're working on
  2. Squash this change down to a single commit

plugins/inputs/t128_graphql/t128_graphql.go Outdated Show resolved Hide resolved
WAN-2321 #time 10m
@shriyanshk128T shriyanshk128T force-pushed the WAN-2321-creating-new-telegraf-inputs-for-event-collector-128-t-and-peer-path branch from 617cc9c to b5f417e Compare September 7, 2023 20:05
@shriyanshk128T shriyanshk128T changed the base branch from release-128tech-1.22 to feature/new-plugins-to-reduce-python-overhead September 7, 2023 20:16
Copy link
Collaborator

@gregschrock gregschrock 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 if it works with the MonitoringAgentCommands.robot

@@ -88,7 +88,6 @@ github.com/Azure/azure-pipeline-go v0.2.3 h1:7U9HBg1JFK3jHl5qmo4CTZKFTVgMwdFHMVt
github.com/Azure/azure-pipeline-go v0.2.3/go.mod h1:x841ezTBIMG6O3lAcl8ATHnsOPVl2bqk7S3ta6S6u4k=
github.com/Azure/azure-sdk-for-go v16.2.1+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=
github.com/Azure/azure-sdk-for-go v51.1.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=
github.com/Azure/azure-sdk-for-go v51.1.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's all this? I don't think any dependencies should have been updated here.

Copy link
Author

Choose a reason for hiding this comment

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

Performed a go mod tidy before pushing!

plugins/inputs/t128_peer_path/t128_peer_path.go Outdated Show resolved Hide resolved
},
{
"node": "node1",
"adjacentAddress": "127.117.97.105",

Choose a reason for hiding this comment

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

indentation is a bit weird here. Also this is to test the one filtering case. Should we add it as a seperate testcase to make it more clear?

Also about the other filtering case? Is that covered?

Copy link
Author

@shriyanshk128T shriyanshk128T Sep 8, 2023

Choose a reason for hiding this comment

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

Yes it is weird but on my vscode it shows fine I will take a look. This test case covers both!

}
}
for _, processedResponse := range processedResponses {
for k, v := range processedResponse.Tags {

Choose a reason for hiding this comment

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

okay - this answers my question about why did we chose to run this as a native collector as opposed to using the graphql input. I guess its fine since this logic is highly custom.

@agrawalkaushik agrawalkaushik merged commit b604e43 into feature/new-plugins-to-reduce-python-overhead Sep 8, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants