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

Add ability to set sql file as query source for postgresql_extensible input #6361

Conversation

Zyqsempai
Copy link
Contributor

@Zyqsempai Zyqsempai commented Sep 6, 2019

Required for all PRs:

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

Fixes #6242

@@ -129,8 +146,18 @@ func (p *Postgresql) Gather(acc telegraf.Accumulator) error {
// We loop in order to process each query
// Query is not run if Database version does not match the query version.
for i := range p.Query {
if p.Query[i].Sqlquery != "" && p.Query[i].Script != "" {
return fmt.Errorf("both 'sqlquery' and 'script' specified, please choose one option")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this and just document that sqlquery takes preference if both are defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion on returning an error vs precedence, though if I was asked to choose I think an error is slightly less prone to mistakes. However, I would like the script loading to be done only once. The easiest way to do this is to move it into a new Init() function:

func (p *Postgresql) Init() error {
    // load the file and save onto the Postgresql struct.
    // return an error if the file is unreadable or configuration is invalid
}

plugins/inputs/postgresql_extensible/README.md Outdated Show resolved Hide resolved
}
defer file.Close()

qyery, err := ioutil.ReadAll(file)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/qyery/query/

Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling still needs fixed.

plugins/inputs/postgresql_extensible/test.sql Outdated Show resolved Hide resolved
@glinton glinton changed the title Added script option to be able set a sql file as a query source Add ability to set sql file as query source for postgresql_extensible input Sep 6, 2019
@glinton glinton added area/postgresql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Sep 6, 2019
plugins/inputs/postgresql_extensible/test.sql Outdated Show resolved Hide resolved
@@ -129,8 +146,18 @@ func (p *Postgresql) Gather(acc telegraf.Accumulator) error {
// We loop in order to process each query
// Query is not run if Database version does not match the query version.
for i := range p.Query {
if p.Query[i].Sqlquery != "" && p.Query[i].Script != "" {
return fmt.Errorf("both 'sqlquery' and 'script' specified, please choose one option")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion on returning an error vs precedence, though if I was asked to choose I think an error is slightly less prone to mistakes. However, I would like the script loading to be done only once. The easiest way to do this is to move it into a new Init() function:

func (p *Postgresql) Init() error {
    // load the file and save onto the Postgresql struct.
    // return an error if the file is unreadable or configuration is invalid
}

@@ -108,6 +111,20 @@ func (p *Postgresql) IgnoredColumns() map[string]bool {
return ignoredColumns
}

func (p *Postgresql) ReadQueryFromFile(filePath string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a non-member function.

@Zyqsempai
Copy link
Contributor Author

@glinton PTAL

plugins/inputs/postgresql_extensible/README.md Outdated Show resolved Hide resolved
}
defer file.Close()

qyery, err := ioutil.ReadAll(file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling still needs fixed.

@Zyqsempai
Copy link
Contributor Author

@glinton Done, PTAL, again;)

Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

Thanks

@danielnelson danielnelson added this to the 1.13.0 milestone Sep 12, 2019
@danielnelson danielnelson merged commit d717e8e into influxdata:master Sep 12, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/postgresql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to read query from file to postgresql_extensible
3 participants