Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

add utf8 validation checks to incoming metrics #1831

Merged
merged 6 commits into from
Jun 2, 2020

Conversation

robert-milan
Copy link
Contributor

@robert-milan robert-milan commented May 21, 2020

We decided to consolidate both UTF8 and Invalid Tag validation into one. It is now called InvalidInput. This will break a few things, including deployments that use reject-invalid-tags. That option no longer exists, now it is just called reject-invalid-input, with a default of true.

For dashboards, the old metric ...metricdata.discarded.invalid_tag no longer exists. It is now called ...metricdata.discarded.invalid_input. Dashboards will need to be updated.

Closes: #1728

@robert-milan robert-milan requested review from replay and Dieterbe May 21, 2020 11:42
@robert-milan
Copy link
Contributor Author

I would still like to add some more unit tests, but I feel pretty confident with this approach

@robert-milan
Copy link
Contributor Author

We are going to change this behavior and instead of using two flags, just use one. I will refactor this to add the discussed changes.

input/input.go Outdated
invalidUtfMD: stats.NewCounterRate32(fmt.Sprintf("input.%s.metricdata.discarded.invalid_utf", input)),
// metric input.%s.metricdata.discarded.invalid_input is a count of times a metricdata was considered invalid due to
// invalid input data in the metric definition. all rejected metrics counted here are also counted in the above "invalid" counter
invalidInputMD: stats.NewCounterRate32(fmt.Sprintf("input.%s.metricdata.discarded.invalid_tag", input)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
invalidInputMD: stats.NewCounterRate32(fmt.Sprintf("input.%s.metricdata.discarded.invalid_tag", input)),
invalidInputMD: stats.NewCounterRate32(fmt.Sprintf("input.%s.metricdata.discarded.invalid_input", input)),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I may have fixed this while you were reviewing it.

@replay
Copy link
Contributor

replay commented Jun 1, 2020

Looks good with one comment.
I think this should be considered a breaking change, because the previously existing setting reject-invalid-tags isn't recognized anymore. So there should be a mention of the change in the "Breaking Changes" section of the CHANGELOG.md

@robert-milan
Copy link
Contributor Author

Looks good with one comment.
I think this should be considered a breaking change, because the previously existing setting reject-invalid-tags isn't recognized anymore. So there should be a mention of the change in the "Breaking Changes" section of the CHANGELOG.md

Yes, I was planning on doing that in a separate PR after I have a new version number to use.

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

LGTM

@robert-milan robert-milan merged commit 79e4709 into master Jun 2, 2020
@robert-milan robert-milan deleted the reject-invalid-utf8 branch June 2, 2020 09:10
Dieterbe added a commit that referenced this pull request Jun 2, 2020
update changelog to include breaking change from #1831
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an option to reject all non-utf8 data
2 participants