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

feat: configurable symbols partitioning #3820

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Jan 7, 2025

Resolves #3262

The PR adds a configuration option (-pyroscopedb.symbols-partition-label) to control how symbols are partitioned.

Currently, we infer the partition key based on the main binary mapping, aiming to create shared partitions for binaries with the same name. This helps optimize memory usage during writes when the same binary runs in different contexts. As a fallback, the service_name label value is used as the symbols partition key.

However, this approach is quite fragile and may result in poor performance during query or compaction operations.

@kolesnikovae kolesnikovae force-pushed the feat/symbols-partition-label branch 2 times, most recently from 526af93 to 01dfd02 Compare January 7, 2025 10:43
Copy link
Collaborator

@korniltsev korniltsev left a comment

Choose a reason for hiding this comment

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

lgtm.

I wonder if this can be a relabeling configuration instead of just one dimension.
it could allow different configurations per tenants
In addition it would allow to merge partitions between similar service_names (cortex-dev-001/compactor, cortext-dev-002/compactor)

pkg/phlaredb/head.go Outdated Show resolved Hide resolved
@kolesnikovae
Copy link
Collaborator Author

I wonder if this can be a relabeling configuration instead of just one dimension.

Yes, it is supposed to be used in conjunction with relabeling rules. For example, you specify your label as symbols partition – __symbols_partition__ and use rules to build the value from other labels. I don't want to duplicate functionality given the existing mechanism could work fine here as is

it could allow different configurations per tenants

This is a service-level parameter, actually. Use of relabeling rules should allow for that.

In addition it would allow to merge partitions between similar service_names (cortex-dev-001/compactor, cortext-dev-002/compactor)

Yes, this can be solved with relabeling rules.

@korniltsev
Copy link
Collaborator

sorry, for some reason I did not think of using an existing relabeling rules

@kolesnikovae kolesnikovae force-pushed the feat/symbols-partition-label branch from 01dfd02 to bffdee6 Compare January 7, 2025 11:36
pkg/phlaredb/head.go Show resolved Hide resolved
@knylander-grafana
Copy link
Contributor

Should we add some doc for this?

@kolesnikovae kolesnikovae marked this pull request as ready for review January 8, 2025 02:17
@kolesnikovae kolesnikovae requested a review from a team as a code owner January 8, 2025 02:17
@kolesnikovae kolesnikovae requested a review from a team as a code owner January 8, 2025 02:43
@kolesnikovae
Copy link
Collaborator Author

@knylander-grafana Thanks for the reminder! I completely forgot to update the generated config docs. I think we're good for now and don't need anything more detailed at this point

@kolesnikovae
Copy link
Collaborator Author

kolesnikovae commented Jan 8, 2025

I've tested the change in one of the dev environments and can confirm that a strict partition-service mapping will cause an increase in memory usage. This was expected, but I'm not sure if we want to allocate more memory / scale-out ingesters

image

In the meantime, I cannot say that it fully addresses the compaction performance issue.

@korniltsev
Copy link
Collaborator

can we try merging some of the partitions? for example mimir-dev-.* => mimir ?

@kolesnikovae
Copy link
Collaborator Author

can we try merging some of the partitions? for example mimir-dev-.* => mimir ?

That would work, but I'd like to avoid overly specific configurations

In the meantime, I cannot say that it fully addresses the compaction performance issue.

Now I'm sure that the environment was malfunctioning. I'm going to repeat the test under more reliable conditions

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.

Improve symbols partitioning heuristics
4 participants