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(parsers.csv): Remove direct checks for the parser type #11825

Merged
merged 7 commits into from
Sep 19, 2022

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Sep 16, 2022

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in [conventional commit format]

partially resolves #11820

This PR introduces a new error (ErrNotEnoughData) for parsers that need to consume more data to output metrics. This is the case for CSV when fed linewise with skipping rows, metadata or other headers involved.

As a consequence, we can remove the direct check for the CSV parser in inputs.tail and inputs.directory_monitor and open the possibility for other parsers to also use this error.

@telegraf-tiger telegraf-tiger bot added area/csv csv parser/serialiser related fix pr to fix corresponding bug plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins labels Sep 16, 2022
@srebhan srebhan requested a review from reimda September 16, 2022 10:40
@telegraf-tiger
Copy link
Contributor

@srebhan srebhan added the regression something that used to work, but is now broken label Sep 16, 2022
@reimda
Copy link
Contributor

reimda commented Sep 19, 2022

This fixes the bug caused by the last two wrapped parsers listed in #11820. It's a fix for the regression in 1.24.0 and should be included in 1.24.1.

When we have more time, we should revisit the approach of using a parser specific error (originally parsers.ErrNotEnoughData in this PR but now named parsers.ErrEOF). If the input plugin is always going to ignore the error, it would be simpler if the parser didn't return an error to begin with. Maybe parsers whose Parse methods calls their own ParseLine method need it?

If we do decide parsers.ErrEOF is needed, we should check the other parsers besides csv and update them to use it. For example, xml could return it if it hasn't received data including a closing tag, and influx could use it when it has received part of a line but not the end of line character.

@reimda reimda merged commit 4897f86 into influxdata:master Sep 19, 2022
reimda pushed a commit that referenced this pull request Sep 19, 2022
dba-leshop pushed a commit to dba-leshop/telegraf that referenced this pull request Oct 30, 2022
@srebhan srebhan deleted the issue_11820 branch November 7, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/csv csv parser/serialiser related fix pr to fix corresponding bug plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins regression something that used to work, but is now broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inputs don't know that parsers are wrapped in runningparser
3 participants