Skip to content

Conversation

aleks-p
Copy link
Contributor

@aleks-p aleks-p commented May 28, 2025

Adds a special meaning to usage groups with placeholders (${labels.label_name}) in their names. When encountered AND there is a match with the provided selector, we replace placeholders with the actual label values.

This allows for a simpler and more powerful configuration of usage groups where this:

    distributor_usage_groups:
      cart-service: '{service_name="cart-service"}'
      user-service: '{service_name="user-service"}'
      order-service: '{service_name="order-service"}'

can be replaced with:

    distributor_usage_groups:
      '${labels.service_name}': '{}'

Benchmark results:

go test -bench=BenchmarkUsageGroups -benchmem ./pkg/validation
goos: darwin
goarch: arm64
pkg: github.com/grafana/pyroscope/pkg/validation
cpu: Apple M2 Pro
BenchmarkUsageGroups_Regular-10            	12253587	        93.08 ns/op	      80 B/op	       1 allocs/op
BenchmarkUsageGroups_Dynamic-10            	 4551181	       265.0 ns/op	     200 B/op	       6 allocs/op

Notes:

  • Both styles of usage groups can co-exist (though if there is overlap, attribution would count towards all groups)

Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure myself yet if I like it. It obviously is pretty aligned with label selectors.

I also could think something more like ${label.service_name} could be quite nice, but then that's something completely different to the label selector.

Edit: Forgot to say: Plus 100 to the feature itself, the current feature is not really usable, esp. as long as its not self service.

Copy link
Contributor Author

@aleks-p aleks-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure myself yet if I like it. It obviously is pretty aligned with label selectors.
I also could think something more like ${label.service_name} could be quite nice, but then that's something completely different to the label selector.

Named placeholders are arguably safer than positional ones. I also think it can fit the usage group model as well, the label selector would be used purely for filtering / segmentation (like today), while the name can be independently controlled, e.g.:

"${label.service_name}": "{}"

I will try this and see how it looks / feels.

@aleks-p aleks-p force-pushed the feat/dynamic_usage_groups branch from 568f510 to 1f75d4a Compare May 29, 2025 14:43
@aleks-p
Copy link
Contributor Author

aleks-p commented May 29, 2025

Switched to named placeholders now. One added benefit is that we can now pre-parse the placeholders (once when the config is read) and can apply it more efficiently. The added overhead decreased quite a bit compared to using regex capture groups.

@aleks-p aleks-p marked this pull request as ready for review May 29, 2025 18:37
@aleks-p aleks-p requested a review from a team as a code owner May 29, 2025 18:37
@kolesnikovae
Copy link
Collaborator

kolesnikovae commented Jun 2, 2025

Just in case: have we considered adding another internal/private label, like __usage_group, to be used in conjunction with relabeling rules?

This would:

  • Unify the different implementations
  • Allow users to explicitly specify usage groups
  • Simplify everything significantly

As I understand it, relabeling rules should be able to do everything that both the proposed and current implementations can do. In 99.(9)% of cases, it's just a simple mapping from service_name.

@aleks-p
Copy link
Contributor Author

aleks-p commented Jun 2, 2025

Just in case: have we considered adding another internal/private label, like __usage_group, to be used in conjunction with relabeling rules?

This would:

  • Unify the different implementations
  • Allow users to explicitly specify usage groups
  • Simplify everything significantly

As I understand it, relabeling rules should be able to do everything that both the proposed and current implementations can do. In 99.(9)% of cases, it's just a simple mapping from service_name.

@kolesnikovae that is an interesting idea. It can work for simpler cases (like the service_name example) but usage groups can and often span multiple dimensions. E.g., a profile can contribute towards the usage statistics for a service, environment, team/department. A single label wouldn't work for this. We could bend the relabelling rules system to accommodate this, but ultimately it serves a different purpose.

@aleks-p aleks-p force-pushed the feat/dynamic_usage_groups branch from b7978df to 58bafa3 Compare June 2, 2025 17:50
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree with Anton's sentiment that this is very close to relabelling rules, I do think we decided to go down this slightly separate route and this an enhancement, which is essential to make the feature usable without developing further automation (like a cron listing service_name and creating usage groups on the back of that).

LGTM

@aleks-p aleks-p force-pushed the feat/dynamic_usage_groups branch from 71405d3 to 389423f Compare June 3, 2025 13:35
@aleks-p aleks-p merged commit 6dd817b into main Jun 3, 2025
24 checks passed
@aleks-p aleks-p deleted the feat/dynamic_usage_groups branch June 3, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants