-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Support compressing metrics payload in OtlpConfig and OtlpHttpMetricsSender #7012
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
Support compressing metrics payload in OtlpConfig and OtlpHttpMetricsSender #7012
Conversation
a1116ea to
b95cf8c
Compare
shakuzen
left a comment
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.
Looks good overall. Thanks for the quick and thorough pull request. I left some feedback.
...ions/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/CompressionMode.java
Outdated
Show resolved
Hide resolved
...entations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpConfig.java
Outdated
Show resolved
Hide resolved
|
Hi @allnightlong, I wanted to follow-up to see if you have time and interest to update this pull request based on the review I left. Let us know. We'd like to get this feature into the upcoming minor release. |
|
@shakuzen thanks for follow up |
d33d473 to
0a03711
Compare
6d9a501 to
4ac8ad2
Compare
Add compressionMode parameter to OtlpMetricsSender.Request to support configurable compression in metrics requests. Signed-off-by: Tigran Kavanosyan <[email protected]>
Wire compression mode configuration through to the HTTP metrics sender to enable configurable compression. Signed-off-by: Tigran Kavanosyan <[email protected]>
Add test coverage for compression mode resolution from configuration and environment variables. Signed-off-by: Tigran Kavanosyan <[email protected]>
Refactor compression mode configuration logic into a dedicated compressionModeFromConfig() method for better code organization. Signed-off-by: Tigran Kavanosyan <[email protected]>
Signed-off-by: Tigran Kavanosyan <[email protected]>
Signed-off-by: Tigran Kavanosyan <[email protected]>
Change enum values from ON/OFF to GZIP/NONE to align with OpenTelemetry specification naming conventions. Signed-off-by: Tigran Kavanosyan <[email protected]>
Add fallback to the generic OTEL_EXPORTER_OTLP_COMPRESSION environment variable when the metrics-specific variable is not set. Signed-off-by: Tigran Kavanosyan <[email protected]>
Signed-off-by: Tigran Kavanosyan <[email protected]>
Fix compression mode tests to use valid enum values (GZIP, NONE) instead of invalid values (ON, OFF). Add test coverage for OTEL_EXPORTER_OTLP_COMPRESSION fallback environment variable and precedence between generic and metrics-specific compression env vars. Signed-off-by: Tigran Kavanosyan <[email protected]>
ad3776d to
9d4ec09
Compare
shakuzen
left a comment
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.
Thanks a lot for the updates. It is looking great. Could we also add an integration test to ensure this actually works with an OTLP endpoint? I think a test case could probably be added to OTelCollectorIntegrationTest with gzip compression enabled in the config. Other than that, updating the documentation would also be good, but if you're not up for that we can take care of it separately.
Adds a test case to OTelCollectorIntegrationTest that verifies metrics export successfully when CompressionMode.GZIP is enabled. Signed-off-by: Tigran Kavanosyan <[email protected]>
54fecbd to
aa4ee50
Compare
|
@shakuzen I've added the integration test - please check it out |
shakuzen
left a comment
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.
Looks good. Thanks for the quick updates. I'll work on merging it tomorrow with some documentation updates.
Resolves #7009