-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Sumo Logic output plugin: carbon2 default to include field in metric #8132
Conversation
…-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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are locks needed here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Removed.
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
fieldRequired for all PRs: