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

Sumo Logic output plugin: carbon2 default to include field in metric #8132

Conversation

pmalek-sumo
Copy link
Contributor

This PR will make Sumo Logic output plugin fallback to include field in metric name when using Carbon2 metrics format without a specified data_format field

Required for all PRs:

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

@pmalek-sumo
Copy link
Contributor Author

Would be super cool if we could also make this into v1.16.0 together with #8115 as that would allow to ship the 'desired' default to users already with the very first version of this plugin.

@sjwang90 sjwang90 added this to the 1.16.0 milestone Sep 21, 2020
@sjwang90 sjwang90 added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 21, 2020
…-to-carbon2-format-including-field-in-metric-pr
@@ -93,6 +94,24 @@ func (s *Serializer) createObject(metric telegraf.Metric) []byte {
return m.Bytes()
}

func (s *Serializer) SetMetricsFormat(f format) {
s.metricsFormatLock.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

are locks needed here?

Copy link
Contributor Author

@pmalek-sumo pmalek-sumo Sep 29, 2020

Choose a reason for hiding this comment

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

In general yes because we don't know how it will be called.
Perhaps calling SetMetricsFormat and IsMetricsFormatUnset from different goroutines will never happen but this is just to be sure.
It's not a hot path so my assumption is that it's ok.
Unless there's a specific concern here I'd leave it this way.

EDIT
Actually, what I'm most concerned is the getMetricsFormat which is called in createObject, which in turn is called in Serialize and SerializeBatch and that I'd like to protect with a lock.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you've specifically seen race conditions, this should be unnecessary. Plugins have their own dedicated goroutines and the interface functions are never called in parallel by Telegraf. We try to avoid this type of defensive programming, as it tends to be confusing and sets a bad example for other plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Removed.

@ssoroka ssoroka merged commit a3a1224 into influxdata:master Sep 29, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants