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: add additional logstash output plugin stats #9707

Merged
merged 6 commits into from
Sep 16, 2021
Merged

fix: add additional logstash output plugin stats #9707

merged 6 commits into from
Sep 16, 2021

Conversation

johnseekins
Copy link
Contributor

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

Logstash exposes some output plugin stats intermittently. We should try and grab those when they exist.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 1, 2021
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.

@johnseekins I have two questions (see comments in the code) regarding your approach. From the code-side everything is fine, but I'm not sure I like the additional metrics. :-)

plugins/inputs/logstash/logstash.go Outdated Show resolved Hide resolved
plugins/inputs/logstash/logstash.go Show resolved Hide resolved
@srebhan srebhan self-assigned this Sep 7, 2021
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.

@johnseekins the code looks code. Can you please add at least one test-case for the new fields to check if they are parsed as expected?

@johnseekins
Copy link
Contributor Author

@johnseekins the code looks code. Can you please add at least one test-case for the new fields to check if they are parsed as expected?

There are actually no existing tests for plugin collection in logstash 7 in the current code. I guess I'll write one for the entire thing?

@srebhan
Copy link
Member

srebhan commented Sep 14, 2021

@johnseekins if you can do that it would be awesome!

@sspaink sspaink added fix pr to fix corresponding bug feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Sep 16, 2021
Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

lgtm!

@sspaink sspaink merged commit f5a3df4 into influxdata:master Sep 16, 2021
reimda pushed a commit that referenced this pull request Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants