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

Make carbon2 metric name more descriptive by including the field tag #8031

Closed
pmalek-sumo opened this issue Aug 25, 2020 · 5 comments
Closed
Labels
feature request Requests for new plugin and for new features to existing plugins

Comments

@pmalek-sumo
Copy link
Contributor

Feature Request

Opening a feature request kicks off a discussion.

Proposal:

Carbon2 metrics format serializer has been introduced in #5345.

The way it was implemented is that the name of the metric sent to 'output' is a bit vague - normally including only the plugin name, e.g. mem or cpu not the field itself e.g. types of memory: free, active or used.

I'd like to propose the ability to configure the serializer to change the metric to include the field (after a _ character.

Current behavior:

The name of the metric sent to 'output' is a bit vague - normally including only the plugin name, e.g. mem or cpu not the field itself e.g. types of memory: free, active or used.
The field itself is serialized as field tag.

This makes the user to additionally specify the field at the destination system where the metrics are being sent.

Desired behavior:

As a Telegraf user, using Carbon2 serializer for my metrics, I'd like to be able to configure the serializer to make the metric name more descriptive i.e. include the field.

So instead of current output like:

metric=mem field=free host=myhostname 44763238400 1597743818

I'd like to propose the following:

metric=mem_free host=myhostname 44763238400 1597743818

Which would also be closer to output from e.g. Prometheus serializer:

mem_free{host="myhostname"} 4.4763238400e+10

Use case:

This would allow users to specify only one field (metric name) at the destination system where the metrics are being sent.

@pmalek-sumo pmalek-sumo added the feature request Requests for new plugin and for new features to existing plugins label Aug 25, 2020
@frankreno
Copy link
Contributor

100% agree with this proposal. The current way it was implemented when I wrote it missed this. Not having a single kv pair to identify the metric is a hindrance in many metric backends. The question to the Telegraf team is this is likely to be considered a breaking change so what is the best way to support such case?

@reimda
Copy link
Contributor

reimda commented Aug 27, 2020

This would be a breaking change for anyone using the serializer so we need a way that defaults to the old behavior. What do you both think about adding a setting called carbon2_format that defaults to "field_separate" and existing behavior but could be set to "metric_includes_field" with the proposed behavior?

Serializer settings are a little messy: add the setting to the Config struct in plugins/serializers/registry.go, then read into it in config/config.go func buildSerializer. Change func plugins/serializers/NewSerializer to pass the new setting into NewCarbon2Serializer.

If that sounds good to you both, @pmalek-sumo will you submit a PR for this?

@pmalek-sumo
Copy link
Contributor Author

@reimda Sure, I can work on that.

What worries me (which is beyond the scope of this PR) is that this will be placed in https://github.com/influxdata/telegraf/blob/master/plugins/serializers/registry.go#L45-L103 which is AFAIK common for all the serializers but doesn't necessarily apply to all.

@pmalek-sumo
Copy link
Contributor Author

@reimda I have prepared a PR with the changes you suggested - #8094.

Let me know if that's OK.

@helenosheaa
Copy link
Member

resolved in #8094

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requests for new plugin and for new features to existing plugins
Projects
None yet
Development

No branches or pull requests

4 participants