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

[exporterhelper] New exporter helper for custom requests #8178

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Aug 7, 2023

Introduce a new exporter helper that operates over client-provided requests instead of pdata. The helper user now has to provide Converter - an interface with a function implementing translation of pdata Metrics/Traces/Logs into a user-defined Request. Request is an interface with only one required function Export.

It opens a door for moving batching to the exporter, where batches will be built from client data format, instead of pdata. The batches can be properly sized by custom request size, which can be different from OTLP. The same custom request sizing will be applied to the sending queue. It will also improve the performance of the sending queue retries for non-OTLP exporters, they don't need to translate pdata on every retry.

This is an implementation alternative to #7874 as suggested in #7874 (comment)

Tracking Issue: #8122

@dmitryax dmitryax requested a review from a team as a code owner August 7, 2023 04:40
@dmitryax dmitryax force-pushed the exporterhelper-v2 branch 2 times, most recently from 43e39ea to 36d90a0 Compare August 7, 2023 05:51
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 82.24% and project coverage change: -0.12% ⚠️

Comparison is base (ebed8dd) 90.27% compared to head (0627049) 90.16%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8178      +/-   ##
==========================================
- Coverage   90.27%   90.16%   -0.12%     
==========================================
  Files         301      302       +1     
  Lines       15596    15733     +137     
==========================================
+ Hits        14080    14186     +106     
- Misses       1227     1254      +27     
- Partials      289      293       +4     
Files Changed Coverage Δ
exporter/exporterhelper/request.go 40.00% <40.00%> (ø)
exporter/exporterhelper/queued_retry.go 90.39% <65.38%> (-4.87%) ⬇️
exporter/exporterhelper/logs.go 83.87% <85.71%> (+3.22%) ⬆️
exporter/exporterhelper/metrics.go 83.87% <85.71%> (+3.22%) ⬆️
exporter/exporterhelper/traces.go 84.04% <85.71%> (+3.09%) ⬆️
exporter/exporterhelper/common.go 91.30% <100.00%> (+0.49%) ⬆️
...porter/exporterhelper/internal/persistent_queue.go 100.00% <100.00%> (ø)
...rter/exporterhelper/internal/persistent_storage.go 89.57% <100.00%> (-3.07%) ⬇️
...xporterhelper/internal/persistent_storage_batch.go 85.36% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Introduce a new exporter helper that operates over client-provided requests instead of pdata. The helper user now has to provide Converter - an interface with a function implementing translation of pdata Metrics/Traces/Logs into a user-defined Request. Request is an interface with only one required function Export.

It opens a door for moving batching to the exporter, where batches will be built from client data format, instead of pdata. The batches can be properly sized by custom request size, which can be different from OTLP. The same custom request sizing will be applied to the sending queue. It will also improve the performance of the sending queue retries for non-OTLP exporters, they don't need to translate pdata on every retry.
Co-authored-by: Alex Boten <alex@boten.ca>
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks ok, just one question and a suggestion

exporter/exporterhelper/metrics.go Outdated Show resolved Hide resolved
exporter/exporterhelper/request.go Show resolved Hide resolved
Co-authored-by: Alex Boten <alex@boten.ca>
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, I prefer this over the previous PR.

@dmitryax dmitryax merged commit a2242f7 into open-telemetry:main Aug 21, 2023
30 of 31 checks passed
@codeboten codeboten added this to the v0.83.0 milestone Aug 24, 2023
@dmitryax
Copy link
Member Author

@codeboten, how did it happen to be added to 0.83.0? It should be part of 0.84.0, right?

@codeboten
Copy link
Contributor

@dmitryax yeah sorry i tagged it incorrectly.... let me revisit the milestone labeling i did manually yesterday

@codeboten codeboten modified the milestones: v0.83.0, next release Aug 25, 2023
dmitryax added a commit to dmitryax/opentelemetry-collector that referenced this pull request Aug 25, 2023
The method was removed from the Request interface in open-telemetry#8178, so the implementation on the test structs is not required anymore
bogdandrutu pushed a commit that referenced this pull request Aug 25, 2023
…tructs (#8286)

The method was removed from the Request interface in
#8178, so
the implementation on the test structs is not required anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants