-
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
JSON serializer emits metric type information #3834
Conversation
Also riding on this patch - ValueType now exposes a String() method that returns the name of the ValueType. This is useful when we print the metric type information (e.g. when serializing) since the reader does not need to be aware of the enum types used within telegraf.
I think we shouldn't emit a type field if the metric is untyped. |
I see your point. But don't you think that its better to have a json schema that is consistent (i.e. the reader can expect the json to have a consistent set of keys) rather than some keys missing sometimes? If I were to write a parser to parse these messages, I'd want this behavior because otherwise I'd be writing a branch in my code to handle the Thoughts? |
It probably depends on what language/library you are using to parse if it helps or not. I will say that most of the metrics in Telegraf are currently untyped because it is only used by the prometheus output. |
I've been thinking about this more and I don't think we should make this change, the main reason is that we want to make changes that will allow setting a type per field, instead of the current model of one type per Metric. This would fix some issues with splitting metrics across lines that otherwise should logically remain together. |
Could you point me to any documentation/writing that could explain this new scheme? (one-metric-many-values). I don't think I follow what you are suggesting. |
I'll try to explain here. Using line protocol, a metric can contain multiple values like:
However, there is no guarantee that all of the values are of the same type, in the above example
In line protocol, which is our primary serialization format, there is actually no reason they can't be on the same line. This is more efficient in terms of space and parsing speed, and also makes processing the data easier since the related values are all on one line. A concrete example of these difficulties is reported in influxdata/kapacitor#1217. Because of this, in the future I'd like to be able to express this as a single metric/line, while still allowing outputs such as the prometheus output to have the required metadata. There are also suggestions to add additional types of metadata that would be on a per field basis. (#3670) I think what would be nice is if we had a new JSON output format that instead of having a hardcoded "shape", would allow the user to specify how they would like the data structured and what data, including metadata, would be included. I'm not sure the exact details of how this would work, so that remains to be fleshed out. |
I think I understand the main goal to split out Type information per Field. However the JSON emitted by this plugin will be restricted to whatever the
So the JSON emitted can simply reflect this. Eventually when each I guess I'm pointing out that feature-A (JSON output) need not be gated on feature-B (type-per-field). This is possible since there are no protocol guarantees provided by the JSON output. The output lines are simply a serialization of the Thoughts? |
While I don't consider the Metric interface to be part of the versioned API, if we add the type to JSON output then we do need to support it until the next major version. While this is not a disqualifier, I'm hesitant to add this when I know it will be changed. That said, I had hoped to add per field types to 1.6, but it turns out doing this without using a lot more memory is a bit trickier than I anticipated and right now it is not a priority. So I think it would be reasonable to move forward with this pull request. |
) | ||
|
||
func (v ValueType) String() string { | ||
return ValueTypeToName[v] |
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.
If ValueType is not in the map don't set a type, or optionally set "untyped".
Closing due to lack of activity, let us know if you want to continue to work on this and we can reopen. |
JSON serializer now includes type information in the string it emits.
Also riding on this patch - ValueType now exposes a String()
method that returns the name of the ValueType. This is useful
when we print the metric type information (e.g. when serializing)
since the reader does not need to be aware of the enum types used
within telegraf.
Required for all PRs: