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

[exporter/loadbalancing] Add the ability to load balance by Metric stream ID #32513

Open
RichieSams opened this issue Apr 18, 2024 · 9 comments
Assignees
Labels

Comments

@RichieSams
Copy link
Contributor

Component(s)

exporter/loadbalancing

Is your feature request related to a problem? Please describe.

The LoadBalancing exporter is a great idea. It currently exposes a number of ways to route metrics coming in:

const (
	traceIDRouting routingKey = iota
	svcRouting
	metricNameRouting
	resourceRouting
)

However, there are many situation where these routing methods can lead to non-uniform distributions of metrics. For example, if datapoints use lots of labels, or med to high cardinality labels. The finest grain routing we can currently do is metric name routing. But if the majority of the datapoints are all the same metric name, then this doesn't help.

Describe the solution you'd like

I would like to propose adding a new value to the routing enum: streamIDRouting. This would route individual datapoints based on their unique ID. We could use the new internal/exp/metrics/identity package: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/internal/exp/metrics/identity/stream.go#L29

I'm willing to be the implementer

Describe alternatives you've considered

No response

Additional context

No response

@RichieSams RichieSams added enhancement New feature or request needs triage New item requiring triage labels Apr 18, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@RichieSams RichieSams changed the title Add the ability to Load balance by Metric stream ID [exporter/loadbalancing] Add the ability to Load balance by Metric stream ID Apr 18, 2024
@RichieSams RichieSams changed the title [exporter/loadbalancing] Add the ability to Load balance by Metric stream ID [exporter/loadbalancing] Add the ability to load balance by Metric stream ID Apr 18, 2024
@JaredTan95
Copy link
Member

@RichieSams Hi, Thanks for the proposal. It's yours

@skandragon
Copy link

I am also needing something similar to this, but likely need to dig a bit deeper and make this per-metric-attribute.

My use case is, I have a custom processor that decorates telemetry with an attribute, placed either on the log, metric, or span. This key is what I want to partition on, and send to each back-end to be gathered and ultimately written to the same s3 output file.

What is the recommended way to add this, as the PR associated with this change seems to be rejected? I know the PR is not sufficient as it only looks at resource attributes, and I would need to look deeper, and I assume split deeper as well.

@jpkrohling
Copy link
Member

I believe @RichieSams will work on this, the PR was closed in favor of breaking it into smaller chunks of work.

@RichieSams
Copy link
Contributor Author

Correct. I already split up the commits, I just got busy at work last week. I'll create new PRs today

@RichieSams
Copy link
Contributor Author

This is the first PR. Just a trivial helper function: #32794

Then there are these 4 commits that will be applied on top. @jpkrohling How do you want me to do them? I can serially make each a PR, get it merged, then go on to the next. Or do you want to combine any?

  1. chore: Add str const variables for the routingKeys RichieSams@bb473e1
  2. [exporter/loadbalacing] Refactor how metrics are split and then re-joined after load-balacing RichieSams@89a14af
  3. [exporter/loadbalancer] Refactor the metrics export benchmarks RichieSams@891bdfa
  4. [exporter/loadbalancer] Add a new routing key: streamID RichieSams@5314b93

jpkrohling pushed a commit that referenced this issue May 29, 2024
**Description:**

This will merge the metrics in mdB into mdA, trying to re-use
resourceMetrics, scopeMetrics, and metric values as possible. This will
be used to help implement the new feature for:
#32513


**Link to tracking Issue:**
#32513
/
#32690

**Testing:**

I created a unit test which tests various scenarios of how merge
behavior should happen

**Documentation:**

The exported function is documented using standard golang style. And
there are comments inside the code to explain what is going on and why

---------

Co-authored-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
Copy link
Contributor

github-actions bot commented Jul 8, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Jul 8, 2024
@RichieSams
Copy link
Contributor Author

This issue is still valid. Currently waiting for this PR to be merged, and then I can create the final PR that will close this issue.

#33676

@github-actions github-actions bot removed the Stale label Jul 10, 2024
@jpkrohling
Copy link
Member

The dependent PR has been merged!

cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jul 11, 2024
…#32794)

**Description:**

This will merge the metrics in mdB into mdA, trying to re-use
resourceMetrics, scopeMetrics, and metric values as possible. This will
be used to help implement the new feature for:
open-telemetry#32513


**Link to tracking Issue:**
open-telemetry#32513
/
open-telemetry#32690

**Testing:**

I created a unit test which tests various scenarios of how merge
behavior should happen

**Documentation:**

The exported function is documented using standard golang style. And
there are comments inside the code to explain what is going on and why

---------

Co-authored-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
jpkrohling pushed a commit that referenced this issue Jul 16, 2024
**Description:** This adds a new routing option for metrics: streamID.
This routes datapoints based on their streamID. That's the unique hash
of all it's attributes, plus the attributes and identifying information
of its resource, scope, and metric data

**Link to tracking Issue:**
#32513

**Testing:** I added to the existing testing suites, testing the new
routing, as well as adding to the benchmark suite

**Documentation:** I updated the README to describe the new routingKey:
`metricID`, and how it works
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants