Allow flow components to provide names via a string instead of river identifier

[erikbaranowski]

Request

There are some additional components to prometheus.exporter.blackbox that are using block labels as argument values. Review and update each that should receive a similar change to grafana/agent#5940.

• prometheus.exporter.cloudwatch
• prometheus.exporter.snmp
  • calls to common.SanitizeIdentifierPanics should be removed in the static to flow converter for this component: https://github.com/grafana/agent/blob/main/converter/internal/staticconvert/internal/build/snmp_exporter.go
• pyroscope.scrape

Use case

River identifiers are fairly restrictive. https://grafana.com/docs/agent/next/flow/config-language/syntax/#identifiers

This prevents someone from having an otherwise valid target name like job-name and is a stumbling block when converting from static mode to flow mode since static mode does not have this restriction.

[github-actions[bot]]

This issue has not had any activity in the past 30 days, so the needs-attention label has been added to it. If the opened issue is a bug, check to see if a newer release fixed your issue. If it is no longer relevant, please feel free to close this issue. The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your issue will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity. Thank you for your contributions!

[rfratto]

Have we considered making River labels less restrictive? They're only restricted because of Flow components, but that could be changed and we can move name validations to the Flow side.

(Sorry @hainenber I know you opened a PR for this proposal as mentioned but I think we need to be aware of alternatives before committing to the specific approach here)

[hainenber]

No worries, Robert. I'm open to proposals.

My (obviously biased) 2cent is that since we've already switched block labels for naming value for 1 of previous component, prometheus.exporter.blackbox and made it into past release, we should dedicate to the remaining cases.

Optionally, we can have a linter to detect and prevent usage of label block.

If River labels are unblocked, previous case should be reverted as well for conformity.

I have no hard stance on either way but feels like there's hidden contexts on how River labels are restricted in the past, thus making the previous PR acceptable for other folks.

[rfratto]

Consistency is definitely a good argument, even if we relax the restriction on River labels. But we should also generally be conscious about breaking change fatigue; I've heard a few concerns that the change for prometheus.exporter.blackbox alone was disruptive to a few deployments.

There might be a third option to avoid more breaking change fatigue:

• Relax the restrictions on River labels to allow special characters
• Make the labels optional for these blocks, rather than required; reintroduce the label in prometheus.exporter.blackbox as an optional label.
• Deprecate the argument in prometheus.exporter.blackbox in favor of the optional label, and schedule it for removal once there's been enough time since the last breaking change.

I don't know if this is the right solution, but I wanted to suggest something to get the ball rolling. Ultimately we have to find a balance between consistency and not burning out users from breaking configurations too frequently.

[ptodev]

I personally like the idea using a string instead of a River identifier, for the reasons I mentioned in a comment:

• It is more in line with how other blocks work too, such as the policy block in the tail sampler.
• It is easier to document than using the block label.
• With a target, the value could come form a component such as local.file. I'm not sure if this is possible with labels.

Using a River identifier looks somewhat cool though. If it was as practical as using a string, I'd have probably preferred it.

I agree that having a release or two when components can be configured in both ways would be nice. It'd allow users to upgrade and rollback without worrying about breaking changes. I hope it wouldn't be too hard to implement. The docs would also need to be clear about which option takes precedence.

[hainenber]

All good points there, Robert. Customer fatigue for breaking changes is definitely unwanted and potentially stop mass adoption 😭

Maybe we can create an issue on upstream grafana/river to explore the idea's feasibility. If not, we can sigh collectively and go with the current approach (maybe) :D