Many v2 integrations do not use identifiers properly

[rfratto]

trafficstars

The following v2 integrations are not using identifiers properly:

• [x] apache_http (#2023)
  • Identifier panics if c.Common.InstanceKey is nil
• [ ] eventhander
  • Identifier ignores value of c.Common.InstanceKey
• [ ] snmp_exporter
  • Identifier ignores value of c.Common.InstanceKey
  • Targets always uses empty instance label

Code example

https://github.com/grafana/agent/blob/f410158210490d70e2fa10ff0fe049245a918087/pkg/integrations/v2/apache_http/apache_http.go#L40-L43

https://github.com/grafana/agent/blob/f410158210490d70e2fa10ff0fe049245a918087/pkg/integrations/v2/apache_http/apache_http.go#L58-L65

Background

Many v2 integrations do not implement the Identifier method correctly. Some also seem to implement InstanceKey, which is not expected of the interface (though it is also implemented incorrectly).

The Identifier method should be implemented for each integration with the following behavior:

• Return c.Common.InstanceKey if it is not nil
• Otherwise, return an integrations-specific default value

The integration-specific default value should be something meaningful to the integration, like the URL of the SUO. If there is no meaningful default value, implementations can return globals.AgentIdentifier.

Always returning *c.Common.InstanceKey will panic if the instance key is not configured, and the instance key is an optional field.

If MetricsIntegration is implemented, the result of calling Identifier should be the value of the instance label for the target. Some v2 integrations also do not do this correctly:

https://github.com/grafana/agent/blob/f410158210490d70e2fa10ff0fe049245a918087/pkg/integrations/v2/snmp_exporter/snmp.go#L33-L65

The instance label should be set to the same value as Identifier, with the same fallback using the globals.

[rfratto]

cc @captncraig @marctc

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed in 7 days if there is no new activity. Thank you for your contributions!

[marctc]

* [x]  `eventhander`
  
  * Identifier ignores value of c.Common.InstanceKey

This integration is not using common.MetricsConfig therefore is not possible to use c.Common.InstanceKey in this context.

[rfratto]

This integration is not using common.MetricsConfig therefore is not possible to use c.Common.InstanceKey in this context.

Ah, you're right. It should probably still provide instance key as a field though so the user can override it like they do for other integrations.

[marctc]

Ah, you're right. It should probably still provide instance key as a field though so the user can override it like they do for other integrations.

Ah, makes sense. Should we reopen this an add follow-up PR?

[rfratto]

Ah, makes sense. Should we reopen this an add follow-up PR?

I can create a new issue 👍