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 carbon2 serializer not falling through to field separate when carbon2_format field is unset #8201

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

pmalek-sumo
Copy link
Contributor

When carbon2_format is unspecified or is specified as empty string there's a missing fallthrough which makes carbon2 serializer not produce metric names at all (noop case in switch).

This PR addresses that and adds a handful of tests for that case.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@@ -38,6 +38,11 @@ func TestSerializeMetricFloat(t *testing.T) {
format: Carbon2FormatFieldSeparate,
expected: fmt.Sprintf("metric=cpu field=usage_idle cpu=cpu0 91.5 %d\n", now.Unix()),
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't make sense to me to duplicate the tests here. Empty doesn't provide different functionality than Separate. It would be simpler to change NewSerializer so that if metricsFormat argument is empty string then it returns a Serializer with metricsformat Carbon2FormatFieldSeparate. Then you could remove Carbon2FormatFieldEmpty altogether. Then you'd only need a single test that NewSerializer("") returns a Serializer with metricsFormat of Carbon2FormatFieldSeparate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sure.
So I've made Carbon2FormatFieldEmpty unexported (i.e. carbon2FormatFieldEmpty) and with this the only place where we need to check for empty string is the constructor as it's impossible to pass format("") even in SetMetricsFormat() because of format being unexported as well.

@reimda reimda merged commit cc089e6 into influxdata:master Oct 7, 2020
@pmalek-sumo pmalek-sumo deleted the fix-empty-carbon2-format-config branch October 7, 2020 19:35
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
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