-
Notifications
You must be signed in to change notification settings - Fork 891
[OTLP] Add support for GZip compression #7055
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
Changes from all commits
f617492
dbbfb44
c18093b
3109a87
3458338
59d7967
09a0c68
3b9d5c8
848ab7a
d2c4f77
870c70a
905e4ca
048b1ac
93dd8df
9f8b3f5
43107f9
c07c97a
4dbc8c3
a0d9f15
64dbf61
04349f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| #nullable enable | ||
| OpenTelemetry.Exporter.OtlpExportCompression | ||
| OpenTelemetry.Exporter.OtlpExportCompression.GZip = 1 -> OpenTelemetry.Exporter.OtlpExportCompression | ||
| OpenTelemetry.Exporter.OtlpExportCompression.None = 0 -> OpenTelemetry.Exporter.OtlpExportCompression | ||
| OpenTelemetry.Exporter.OtlpExporterOptions.Compression.get -> OpenTelemetry.Exporter.OtlpExportCompression | ||
| OpenTelemetry.Exporter.OtlpExporterOptions.Compression.set -> void |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| using System.Net.Http; | ||
| #endif | ||
| using System.Diagnostics.Tracing; | ||
| using System.IO.Compression; | ||
| using System.Net.Http.Headers; | ||
|
|
||
| namespace OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient; | ||
|
|
@@ -54,4 +55,34 @@ public override ExportClientResponse SendExportRequest(byte[] buffer, int conten | |
| return new ExportClientHttpResponse(success: false, deadlineUtc: deadlineUtc, response: null, exception: ex); | ||
| } | ||
| } | ||
|
|
||
| protected override HttpContent CreateHttpContent(byte[] buffer, int contentLength) | ||
| { | ||
| if (!this.CompressionEnabled) | ||
| { | ||
| return base.CreateHttpContent(buffer, contentLength); | ||
| } | ||
|
|
||
| #if NET | ||
| var compressedStream = new PooledBufferStream(); | ||
| #else | ||
| var compressedStream = new MemoryStream(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets leave a todo for a future optimization to see if we can avoid allocation by renting/pooling arrays.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What specifically were you thinking of?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've got an implementation of the third option locally, I'm just testing it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/communityToolkit/dotnet has quite a few implementations.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We wouldn't be able to take a dependency on those.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't recommend that for this library either. But it has quite a few with unit tests. I don't see a problem grabbing the code from one that fits here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took a quick look, and didn't see an "obvious" one that grabbed from the array pool, plus they seemed to have a bunch of extra interfaces I'd have to prune out. As I'd already written an implementation when you commented, I'll stick with that unless there's a compelling reason to replace it with something adapted and vendored from there.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops! I meant to leave a TODO for revisiting this in a future PR to keep the initial version easier to review. My suggestion is to avoid the optimization in this PR, so review can focus on just the feature, and then a follow up where we can focus on pooling/efficiency part! Sorry I was not very clear initially!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I've already done the work either I can remove it and just immediately open a PR as a draft that's the same plus 2 extra files to rebase later and then re-run the benchmarks without the optimization, or we can just leave it in. |
||
| #endif | ||
|
|
||
| using (var gzipStream = new GZipStream(compressedStream, CompressionLevel.Fastest, leaveOpen: true)) | ||
| { | ||
| gzipStream.Write(buffer, 0, contentLength); | ||
| } | ||
|
|
||
| compressedStream.Position = 0; | ||
|
|
||
| OpenTelemetryProtocolExporterEventSource.Log.CompressedHttpPayload("gzip", contentLength, compressedStream.Length); | ||
|
|
||
| var content = new StreamContent(compressedStream); | ||
|
cijothomas marked this conversation as resolved.
|
||
|
|
||
| content.Headers.ContentType = this.MediaTypeHeader; | ||
| content.Headers.Add("Content-Encoding", "gzip"); | ||
|
|
||
| return content; | ||
| } | ||
| } | ||
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.
nit: maybe mention the signal specific env vars too here.
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.
also, if we can update the OTLP's readme also, that'd better.
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.
Good point - I forgot about the README.
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.
README updated.