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

feat(inputs.logstash): Record number of failures #9185

Merged
merged 16 commits into from
Jul 11, 2022

Conversation

bartigor
Copy link
Contributor

  • Updated associated README.md.
  • Wrote appropriate unit tests.

added a metric for counting errors by plugins on logstash input plugin

resolves #

I added the collection of metrics for the number of failures in the Logstash Input Plugin

@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@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 Apr 24, 2021
@telegraf-tiger
Copy link
Contributor

🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@bartigor
Copy link
Contributor Author

!signed-cla

Copy link
Contributor

@ivorybilled ivorybilled left a comment

Choose a reason for hiding this comment

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

Makes sense, just curious about adding it as a new metric with the same name vs adding a field to the existing metric, is that necessary?

@bartigor
Copy link
Contributor Author

About adding a new field to the existing metric: where do you sugest to add it?
I made a new metric becaus 'failues' are not on the same level in Logstash stats api https://www.elastic.co/guide/en/logstash/current/node-stats-api.html#pipeline-stats

@sspaink
Copy link
Contributor

sspaink commented Jun 3, 2022

@bartigor sorry you haven't received a response in a while, are you still interested in working on this pull request? I think having it in a separate metric will probably be fine. I was wondering how you were planning on using the number of failures, wouldn't you need more context around those failures to make them a useful metric?

@sspaink sspaink added the waiting for response waiting for response from contributor label Jun 3, 2022
@bartigor
Copy link
Contributor Author

bartigor commented Jun 3, 2022

I use this metric in my production to detect microservices that have changed the log format. So far, just the number of failures is enough for me? and if they have grown sharply, then it means that it is correctly parsed. You can probably expand the context, but so far I have not thought about how to do it better.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jun 3, 2022
@sspaink sspaink changed the title Logstash plugin count failures feat(inputs.logstash): Record number of failures Jun 3, 2022
@sspaink
Copy link
Contributor

sspaink commented Jun 3, 2022

Thank you for the context, if you could rebase your pull request against the latest master branch to have CI run again that would be great.

@bartigor
Copy link
Contributor Author

bartigor commented Jun 3, 2022

It didn’t work to automatically merge master, a long time passed, and the master went ahead. I'll fix it when the time comes.

@sspaink sspaink 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 Jun 29, 2022
@sspaink
Copy link
Contributor

sspaink commented Jul 6, 2022

@bartigor looks like the tests are failing due to some syntax issues, do you have time to resolve them?

@sspaink sspaink added the waiting for response waiting for response from contributor label Jul 6, 2022
@bartigor
Copy link
Contributor Author

bartigor commented Jul 8, 2022

I fixed it, everything seems to compile and pass

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Jul 8, 2022
@sspaink
Copy link
Contributor

sspaink commented Jul 8, 2022

@bartigor sorry seems the unit tests didn't get triggered, I triggered them manually and they are failing with:

[ERROR] gofmt has found errors in the following files:
plugins/inputs/logstash/logstash.go

Run make fmt to fix them.
make: *** [Makefile:152: fmtcheck] Error 1

@sspaink sspaink added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Jul 8, 2022
@bartigor
Copy link
Contributor Author

bartigor commented Jul 9, 2022

fixed

@reimda reimda merged commit 8b3cab8 into influxdata:master Jul 11, 2022
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 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.

4 participants