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.

Sign up for GitHub

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

Jump to bottom

Proposal: integrations-next: Consider dropping _exporter suffix from integrations #1231

Closed
rfratto opened this issue Jan 6, 2022 · 14 comments · Fixed by #1508
Closed

Proposal: integrations-next: Consider dropping _exporter suffix from integrations #1231

rfratto opened this issue Jan 6, 2022 · 14 comments · Fixed by #1508
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. proposal Proposal or RFC
Milestone
v0.24.0

Comments

@rfratto
Copy link
Member

rfratto commented Jan 6, 2022 •
edited

Our integration names aren't really consistent. To mention a few:

  • agent
  • cadvisor
  • node_exporter
  • mysqld_exporter

The names come from the project which is being embedded into an integration, though I don't know if a user necessarily cares about mysqld_exporter existing.

Since we have the opportunity to make large swaths of breaking changes in integrations-next, we should consider dropping the _exporter suffix from integrations.

The new name of integrations should be obvious for many of them, like changing mysqld_exporter to mysqld for "the mysqld integration".

There would be two problems, though:

  • What would node_exporter be renamed to? node seems weird. node_exporter would either be renamed to unix or left alone as node_exporter as an exception
  • The _exporter suffix is a nice indication that it's a metrics-based integration. Do we care that it won't be obvious any more what enabling a mysqld integration does for telemetry?

cc @mattdurham @rgeyer @gaantunes
@rfratto rfratto mentioned this issue Jan 6, 2022
Merged
3 tasks
@mattdurham
Copy link
Collaborator

I like dropping the exporter suffix

@rgeyer
Copy link
Contributor

rgeyer commented Jan 7, 2022

I'm fully in agreement with this. When I was working on cadvisor the very same thing occurred to me. Specifying _exporter is clumsy and redundant, and will become increasingly irrelevant as we introduce other integrations which aren't exporters, or aren't even metrics.

@rfratto
Copy link
Member Author

rfratto commented Jan 7, 2022

What would we rename node_exporter to though? node? That could be confused with node.js

@rgeyer
Copy link
Contributor

rgeyer commented Jan 8, 2022

What would we rename node_exporter to though? node? That could be confused with node.js

I'd vote for linux_node. We use that for naming in some other places, including the integration itself.

@rfratto
Copy link
Member Author

rfratto commented Jan 10, 2022 •
edited

linux_node isn't completely prescriptive of what node_exporter can do, though 😞 node_exporter supports BSDs and macOS systems equally as well, and I'm not sure we should have a macos and/or bsd integrations that are just thin wrappers around node_exporter for the sake of it.

What about unix? node_exporter's readme describes itself as an exporter for *NIX systems. (I recognize unix is a subset of *NIX but nix would make people think of https://nixos.org/. We can say "unix-like" in the docs and elaborate what that means)

@rgeyer
Copy link
Contributor

rgeyer commented Jan 10, 2022

Yep, I agree. unix and "Unix like" are more accurate and descriptive. That gets my vote :)

@tpaschalis
Copy link
Member

I'd also agree with dropping the _exporter suffix. unix sounds nice, but my 2c here, node_exporter might warrant an exclusion to the rule to keep its former name.

It's probably the simplest useful integration when toying around with the agent, thus should be easily recognizable.

@v-zhuravlev
Copy link
Contributor

v-zhuravlev commented Feb 16, 2022 •
edited

I would like to see _exporter suffix stay for those integrations that use prometheus integrations as go modules. As experienced prom user, I can have clear understanding that those integrations are tightly coupled with those exporters just by looking at the integration name. And what metrics can be expected, how those integrations are configured...
One of the use cases of Grafana agent is that a user gains the convenience to deploy single binary instead of bunch of different exporters he/she used to deploy separately. So I think it is great if it is visible that Grafana agent is a drop-in replacement for those exporters embedded.

@rfratto rfratto added this to the v0.24.0 milestone Feb 17, 2022
@rfratto
Copy link
Member Author

rfratto commented Feb 23, 2022

One issue with making this change is whether the job label for metrics changes (i.e., job="intgrations/mysql_exporter" to job="integrations/mysql").

cc @rgeyer this would probably need coordination with the integration dashboards so they support either the old or new job label (or we change the name but not the job label, but that could be confusing for other reasons)

@rgeyer
Copy link
Contributor

rgeyer commented Feb 23, 2022

One issue with making this change is whether the job label for metrics changes (i.e., job="intgrations/mysql_exporter" to job="integrations/mysql").

cc @rgeyer this would probably need coordination with the integration dashboards so they support either the old or new job label (or we change the name but not the job label, but that could be confusing for other reasons)

This would be fine from the integration standpoint. We allow the user to select the job and instance which "matches" a metric we expect from the exporter (usually up, actually). So the dashboards would continue to work without modification.

However, the agent config which the integrations-api generates would need to be updated to reflect the change.

@rfratto
Copy link
Member Author

rfratto commented Mar 3, 2022 •
edited

So the dashboards would continue to work without modification.

At the very least, Grafana Cloud's node exporter dashboards looks specifically for job="integrations/node_exporter", so that would need updating.

Should we maybe leave the job label untouched? cc @mattdurham @tpaschalis (Which would not break any dashboards but might confuse users if the labels are desynced)

@rfratto
Copy link
Member Author

rfratto commented Mar 3, 2022

At the very least, Grafana Cloud's node exporter dashboards looks specifically for job="integrations/node_exporter", so that would need updating.

Should we maybe leave the job label untouched? cc @mattdurham @tpaschalis (Which would not break any dashboards but might confuse users if the labels are desynced)

Turns out I had a very old version of the dashboard and none of the new ones assume the job label, so this is a safe change to make.

@rfratto
Copy link
Member Author

rfratto commented Mar 3, 2022

Right now the only two questions seem to be:

  • Should we do this at all?
  • Should node_exporter stay the same or be renamed to unix
  • Should mysqld_exporter be renamed to mysqld or mysql?
  • Should we do a hard breaking change of the names for integrations-next or continue to support the old names for a while?

@rgeyer
Copy link
Contributor

rgeyer commented Mar 3, 2022

Just my votes.. I don't have super strong opinions here tho.

Right now the only two questions seem to be:

  • Should we do this at all?
    Yes
  • Should node_exporter stay the same or be renamed to unix
    I prefer unix, we can document/update as needed downstream
  • Should mysqld_exporter be renamed to mysqld or mysql?
    I prefer mysqld
  • Should we do a hard breaking change of the names for integrations-next or continue to support the old names for a while?
    Loathe as I am to say, we should probably deprecate, and remove after a couple revisions, rather than a hard breaking change.

rfratto added a commit to rfratto/agent that referenced this issue Mar 16, 2022
@rfratto
integrations-next: drop _exporter suffix for integrations
0b69736 
This commit changes the name of many `integrations-next` integrations to
represent what type of data they generate rather than the projects they
use internally to generate the data.

The following integrations have changed names:

* `consul_exporter` is now `consul`
* `dnsmasq_exporter` is now `dnsmasq`
* `elasticsearch_exporter` is now `elasticsearch`
* `github_exporter` is now `github`
* `kafka_exporter` is now `kafka`
* `memcached_exporter` is now `memcached`
* `mongodb_exporter` is now `mongodb`
* `mysqld_exporter` is now `mysql`
* `postgres_exporter` is now `postgres`
* `process_exporter` is now `process`
* `redis_exporter` is now `redis`
* `statsd_exporter` is now `statsd`
* `windows_exporter` is now `windows`

It is worthwhile pointing out oddities this introduces:

* `node_exporter` remains unchanged
* `mysqld_exporter` has been changed to `mysql` and not `mysqld`
* `process_exporter` has been changed to `process`, which may be
  confusing to people.

Integrations that supported multiple instances still require the
`_configs` suffix in the YAML; `integrations.mysqld_exporter_configs` is
now `integrations.mysql_configs`.

Closes grafana#1231.
@rfratto rfratto mentioned this issue Mar 16, 2022
Merged
rfratto added a commit that referenced this issue Mar 17, 2022
@rfratto
integrations-next: drop _exporter suffix for integrations (#1508)
8d09087 
This commit changes the name of many `integrations-next` integrations to
represent what type of data they generate rather than the projects they
use internally to generate the data.

The following integrations have changed names:

* `consul_exporter` is now `consul`
* `dnsmasq_exporter` is now `dnsmasq`
* `elasticsearch_exporter` is now `elasticsearch`
* `github_exporter` is now `github`
* `kafka_exporter` is now `kafka`
* `memcached_exporter` is now `memcached`
* `mongodb_exporter` is now `mongodb`
* `mysqld_exporter` is now `mysql`
* `postgres_exporter` is now `postgres`
* `process_exporter` is now `process`
* `redis_exporter` is now `redis`
* `statsd_exporter` is now `statsd`
* `windows_exporter` is now `windows`

It is worthwhile pointing out oddities this introduces:

* `node_exporter` remains unchanged
* `mysqld_exporter` has been changed to `mysql` and not `mysqld`
* `process_exporter` has been changed to `process`, which may be
  confusing to people.

Integrations that supported multiple instances still require the
`_configs` suffix in the YAML; `integrations.mysqld_exporter_configs` is
now `integrations.mysql_configs`.

Closes #1231.
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. proposal Proposal or RFC
Projects
None yet
Milestone
v0.24.0
Development

Successfully merging a pull request may close this issue.

integrations-next: drop _exporter suffix for integrations rfratto/agent
5 participants
@rgeyer @rfratto @mattdurham @v-zhuravlev @tpaschalis