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/kafka] Impelement partitioning for OTLP metrics #31315

Merged
merged 49 commits into from
Apr 29, 2024

Conversation

SHaaD94
Copy link
Contributor

@SHaaD94 SHaaD94 commented Feb 19, 2024

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

Copy link

linux-foundation-easycla bot commented Feb 25, 2024

@SHaaD94
Copy link
Contributor Author

SHaaD94 commented Feb 27, 2024

Hello, @dmitryax. Any chance you could have a look at this PR?
Thank you!

@SHaaD94 SHaaD94 changed the title Impelement partitioning for OTLP metrics [exporter/kafka] Impelement partitioning for OTLP metrics Mar 14, 2024
@SHaaD94 SHaaD94 requested a review from crobert-1 March 18, 2024 06:42
@SHaaD94 SHaaD94 force-pushed the balance-metrics-by-resources branch from 37f375d to b66537a Compare April 26, 2024 11:43
@SHaaD94
Copy link
Contributor Author

SHaaD94 commented Apr 26, 2024

I bumped go version to 1.21.9 to fix (it is present in the main branch as well)

Vulnerability #1: GO-2024-2687
    HTTP/2 CONTINUATION flood in net/http
  More info: https://pkg.go.dev/vuln/GO-2024-2687
  Standard library
    Found in: net/http@go1.21.8
    Fixed in: net/http@go1.21.9
    Example traces found:
      #1: config.go:10:2: kafkaexporter.init calls sarama.init, which eventually calls http.CanonicalHeaderKey
      #2: jaeger_marshaler.go:11:2: kafkaexporter.init calls model.init, which eventually calls http.Header.Del
      #3: jaeger_marshaler.go:11:2: kafkaexporter.init calls model.init, which eventually calls http.Header.Get
      #4: kafka_exporter.go:151:25: kafkaexporter.kafkaLogsProducer.Close calls sarama.syncProducer.Close, which eventually calls http.bodyEOFSignal.Read
      #5: pdata_marshaler.go:70:32: kafkaexporter.pdataMetricsMarshaler.Marshal calls pdatautil.MapHash, which eventually calls http.bodyLocked.Read
      #6: kafka_exporter.go:40:20: kafkaexporter.kafkaErrors.Error calls fmt.Sprintf, which eventually calls http.http2ConnectionError.Error
...

Please let me know if it is a wrong way to fix it, thanks

@SHaaD94 SHaaD94 requested a review from dmitryax April 29, 2024 03:22
exporter/kafkaexporter/pdata_marshaler.go Outdated Show resolved Hide resolved
Comment on lines +59 to 62
// Key configures the pdataMetricsMarshaler to set the message key on the kafka messages
func (p *pdataMetricsMarshaler) Key() {
p.keyed = true
}
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

@dmitryax dmitryax merged commit cec2153 into open-telemetry:main Apr 29, 2024
169 of 170 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 29, 2024
@SHaaD94 SHaaD94 deleted the balance-metrics-by-resources branch April 30, 2024 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants