Skip to content

Remove error return type from metric.New method#9116

Merged
ivorybilled merged 6 commits intomasterfrom
removeErrorFromMetricNew
Apr 13, 2021
Merged

Remove error return type from metric.New method#9116
ivorybilled merged 6 commits intomasterfrom
removeErrorFromMetricNew

Conversation

@ivorybilled
Copy link
Copy Markdown
Contributor

the metric.New method never returns an error, which led to a lot of unnecessary checks in the code. This PR removes the error return type from it and refactors the 94 files that referenced it.

Copy link
Copy Markdown
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

Copy link
Copy Markdown
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

@@ -227,11 +227,7 @@ func (p *parser) readDWMetrics(metricType string, dwms interface{}, metrics []te
parsed, err := p.seriesParser.Parse([]byte(measurementName))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm wondering if this error needs to be returned instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like it's handled in the if statement two lines below, I'm unsure if that's what you meant though

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah mainly just thought why are we nil checking it but then not either returning the error or logging it. I'm aware this isn't related to what you've changed so probably fine to leave it as is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, although as it's out of scope for this PR I'll probably leave it. and thanks for reviewing!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah this logic is definitely bad. there's a panic hiding in here too. You don't have to fix it in this PR, but please circle back to it and/or open an issue

@ivorybilled
Copy link
Copy Markdown
Contributor Author

!retry-checks

@ivorybilled ivorybilled merged commit 842a788 into master Apr 13, 2021
@ivorybilled ivorybilled deleted the removeErrorFromMetricNew branch April 13, 2021 18:40
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Aug 28, 2025
* Remove error return type from metric.New method.

* Formatting changes for linter + gofmt

* Additional linter fixes.

* More linter fixes.

* Linter fix.

* address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants