Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Splunk Metrics serializer #4339
Splunk Metrics serializer #4339
Changes from 1 commit
135799b
5dc75f4
864ba51
49a3c28
b53b2c3
8c4535f
30cad33
cc29b29
ac56a95
a3c8374
9c01483
1bb4e2e
67072ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Splunk only supports seconds. The serializer should not be using
config.TimestampUnits
.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.
Splunk absolutely support sub second timestamps. From the default datetime.xml:
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.
I didn't say it doesn't. It only supports seconds as the unit. It supports more than that as the precision. Unit != precision.
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.
spelling
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.
again, leading
#
isn't necessaryThere 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.
SerializeBatch
seems like it's just a reinvention ofjson.Encoder
. It might be better to use the existing standard lib functionality. It should also simplify your code a lot.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.
The error should be logged somewhere when not nil. Otherwise metrics are going to be getting dropped and the user won't have any idea why.
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.
It would be better if
serialized
were a byte slice. Every time you append to a string, a new string has to be allocated. If your batches are large (in terms of count or size), this can suck up a lot of memory. With byte slice, an allocation is performed only if the length of the slice exceeds the capacity.Ditto for other places where this is stored in a string (e.g. line 23).
Also I'm not seeing much point of the
objects
slice. You should be able to append toserialized
directly.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.
We do a bad job of explaining what is guaranteed in a telegraf.Metric, and actually as I write this I notice some additional checks I need to add.
Here is a brief rundown of things you may or may not need to check:
The part about tag/field keys not being empty strings is not true right now, but after writing this I am going to ensure this is the case in 1.8.
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.
Thanks, this actually caused me to re-examine the the serializer with several different inputs, and I found a case in which metrics were lost (dropped), so I'm refactoring some of the code to deal with that. (Also, it's been a crazy week...so hope to get this done over the next few days.)
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.
This is not entirely accurate. Splunk's http event collector is not line-oriented. It's object oriented. You can shove multiple JSON objects on a single line and it's happy to consume them.
See this example: https://docs.splunk.com/Documentation/Splunk/7.1.1/Data/HTTPEventCollectortokenmanagement#Send_multiple_events_to_HEC_in_one_request
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.
I agree, it's not line oriented, but it doesn't support reading an array of JSON objects as metrics. This is OK:
This is not OK:
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.
My point was that developer documentation should be accurate. We shouldn't tell the developers it's "one metric per line" if it's not.
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.
I personally don't think this is a good structure to follow. This will prevent using this serializer with pretty much every single input plugin, as none of them follow this format. I think the implementation over on #4185 has a much more flexible implementation.
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.
The comment is wrong, it should be
time
, not_time
(I'll fix), butmetric_name
and_value
are required fields for the metrics store. Naming the fields this way means you don't need custom props.conf or transforms.conf. In fact this is the same naming convention that is used in the PR you reference. The point of this serializer is to format the metrics into this format so that it can be used with the generic, well tested, HTTP output. Or the file output if you're running telegraf on a machine that is running a Splunk forwarder.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.
Use metric.FieldList() so that no allocation is performed.
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.
Part of the refactoring mentioned below (thank for the suggestion)
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.
I think I see what you're intending to do here, but this won't result in the correct behavior. https://play.golang.org/p/VEDBzbF0j0e
If you want to do math between 2 ints, and get a float result, you need to convert the int to float before the arithmetic.
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.
Picking on the string vs byte slice thing some more, you're converting a byte slice to a string, and then later on converting that string back into a byte slice (up on line 50). It would use less memory to keep it as a byte slice.