-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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/kafka] Impelement partitioning for OTLP metrics #31315
[exporter/kafka] Impelement partitioning for OTLP metrics #31315
Conversation
f8b2244
to
eb4f364
Compare
eb4f364
to
1380a2a
Compare
|
b6b8177
to
1380a2a
Compare
Hello, @dmitryax. Any chance you could have a look at this PR? |
…y-collector-contrib into balance-metrics-by-resources
…y-collector-contrib into balance-metrics-by-resources
…y-collector-contrib into balance-metrics-by-resources
37f375d
to
b66537a
Compare
I bumped go version to 1.21.9 to fix (it is present in the main branch as well)
Please let me know if it is a wrong way to fix it, thanks |
Co-authored-by: Curtis Robert <crobert@splunk.com>
…y-collector-contrib into balance-metrics-by-resources
…ntelemetry-collector-contrib into balance-metrics-by-resources
// Key configures the pdataMetricsMarshaler to set the message key on the kafka messages | ||
func (p *pdataMetricsMarshaler) Key() { | ||
p.keyed = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This control is super confusing. It's unclear what Key
is doing and why it's needed. I believe the flag can be simply sent to the newPdataMetricsMarshaler
constructor. And there is no need to expose the KeyableMetricsMarshaler
API here.
I understand that you are doing it consistently with what's already done for tracing. We can keep it as is, but should be refactored later for both metrics and traces. Feel free to submit an issue if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't mind, I would prefer to leave it as is for now and refactor it in next PR. I am planing to raise it shortly for logs partitioning by resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I suggesting
Description: Add resource attributes based partitioning for OTLP metrics
In our backend we really need an ability to distribute metrics based on resource attributes.
For this I added additional flag to the configuration.
Some code from traces partitioning by traceId reused.
Judging by issues, this feature is anticipated by several more people.
Link to tracking Issue: 31675
Additionally this feature was menioned in these issues: 29433, 30666
Testing:
Added tests for hashing utility.
Added tests for marshalling and asserting correct keys and the number of messages.
Tested locally with host metrics and chained OTLP metrics receiver.
Documentation:
Changelog entry
Flag is added to the doc of KafkaExporter