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

fix: Update prometheus parser to be a new style parser plugin #11440

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

reimda
Copy link
Contributor

@reimda reimda commented Jun 30, 2022

resolves #11361

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Jun 30, 2022
@reimda reimda added ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. and removed ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Jul 1, 2022
@reimda reimda marked this pull request as draft July 1, 2022 22:31
@reimda
Copy link
Contributor Author

reimda commented Jul 1, 2022

There are a couple prometheus settings that don't seem to do anything. I'm not sure if they are mistakes or if they were valid settings in the past and were removed, then intentionally left in so they will be ignored for backward compatibility.

  • prometheus_ignore_timestamp
    • in missingTomlField
  • prometheus_sort_metrics
    • in missingTomlField
    • in the prometheus serializer readme
    • in plugins/serializers/registry.go but unused elsewhere in code

I removed these in this PR but I'm having second thoughts. I need to figure out what to do with these before we merge this PR.

Copy link
Member

@srebhan srebhan 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 to me. Just one comment regarding Init.
I think your option-removal is safe, but I would prefer to do the serializer one in an extra PR to be able to easily revert it if necessary.

Comment on lines 191 to 194
func (p *Parser) Init() error {
return nil
}

func (p *Parser) InitFromConfig(config *parsers.Config) error {
p.IgnoreTimestamp = config.PrometheusIgnoreTimestamp
return p.Init()
}
Copy link
Member

Choose a reason for hiding this comment

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

You can kill Init here and just return nil

Suggested change
func (p *Parser) Init() error {
return nil
}
func (p *Parser) InitFromConfig(config *parsers.Config) error {
p.IgnoreTimestamp = config.PrometheusIgnoreTimestamp
return p.Init()
}
func (p *Parser) InitFromConfig(config *parsers.Config) error {
p.IgnoreTimestamp = config.PrometheusIgnoreTimestamp
return nil
}

@srebhan srebhan self-assigned this Jul 5, 2022
@reimda
Copy link
Contributor Author

reimda commented Jul 5, 2022

I updated this to touch only the prometheus parser and to remove the empty Init()

@reimda reimda marked this pull request as ready for review July 5, 2022 15:55
Copy link
Member

@srebhan srebhan 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 to me. Thanks for migrating this one @reimda!

@srebhan srebhan added ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins labels Jul 6, 2022
@reimda reimda merged commit c25f9a0 into master Jul 6, 2022
@reimda reimda deleted the fix-11361 branch July 6, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate prometheus to new parser structure
3 participants