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] OTLP file exporter #2540

Merged
merged 32 commits into from
Apr 3, 2024
Merged

Conversation

owent
Copy link
Member

@owent owent commented Feb 18, 2024

Fixes #2482

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@owent owent requested a review from a team February 18, 2024 09:12
@owent owent changed the title [WIP] OTLP file exporter. OTLP file exporter. Feb 19, 2024
@owent
Copy link
Member Author

owent commented Feb 19, 2024

This PR is ready to be reviewed now. @open-telemetry/cpp-approvers @perhapsmaple

@perhapsmaple
Copy link
Contributor

Hi @owent, thanks for doing this. This is a very nice and well done PR. I have a couple of questions for now:

  1. I'm not very familiar with this rolling strategy. Most log tailers like Promtail and Fluentbit recommend the Rename and Create rotation strategy. Has this rotation strategy been tested with any existing log tailer? See here for more information: https://grafana.com/docs/loki/latest/send-data/promtail/logrotation/

  2. I know this is probably a bit out of scope of the opentelemetry specification, but is it possible to make a base class like FileBackend so that users can implement their own rotation strategies if required. The OtlpFileBackend is very well designed and configurable, but might not fit everyone's use case.

We will have to test it with the otlpjsonfilereceiver from opentelemetry-collector-contrib to make sure that everything works properly. I'm a little busy for the next couple days but I can help you with this after.

Thanks

@owent
Copy link
Member Author

owent commented Feb 20, 2024

Hi @owent, thanks for doing this. This is a very nice and well done PR. I have a couple of questions for now:

  1. I'm not very familiar with this rolling strategy. Most log tailers like Promtail and Fluentbit recommend the Rename and Create rotation strategy. Has this rotation strategy been tested with any existing log tailer? See here for more information: https://grafana.com/docs/loki/latest/send-data/promtail/logrotation/
  2. I know this is probably a bit out of scope of the opentelemetry specification, but is it possible to make a base class like FileBackend so that users can implement their own rotation strategies if required. The OtlpFileBackend is very well designed and configurable, but might not fit everyone's use case.

We will have to test it with the otlpjsonfilereceiver from opentelemetry-collector-contrib to make sure that everything works properly. I'm a little busy for the next couple days but I can help you with this after.

Thanks

Thanks.

  1. I want to keep the default implementation simple. This rotation strategy is tested in our internal collector agent which is also based on otel-collector.
  2. It's a good idea to leave a base class to let users to use their own log sink, maybe spdlog, boost.log or other components.I will add it later. You can review it anytime when you have time. Thanks.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Very good work, as we are used to.

See some minor changes or comments.

CMakeLists.txt Outdated Show resolved Hide resolved
exporters/otlp/src/otlp_file_client.cc Outdated Show resolved Hide resolved
exporters/otlp/src/otlp_file_client.cc Outdated Show resolved Hide resolved
@marcalff marcalff added the pr:please-review This PR is ready for review label Mar 21, 2024
@marcalff
Copy link
Member

Need to resolve my 3 comments, and need a second reviewer: adding the please-review label.

@owent
Copy link
Member Author

owent commented Mar 25, 2024

Need to resolve my 3 comments, and need a second reviewer: adding the please-review label.

Done

@lalitb
Copy link
Member

lalitb commented Mar 29, 2024

Nice work. Can we document this exporter as an section in README.md ?

@owent
Copy link
Member Author

owent commented Apr 2, 2024

Nice work. Can we document this exporter as an section in README.md ?

Sorry for the missing. I will add documents tomorrow several days later.
Sorry, I'm too busy recently.I will commit and push documents together with codes if there are more feedbacks about this PR.

@marcalff marcalff added ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) and removed pr:please-review This PR is ready for review labels Apr 3, 2024
@marcalff
Copy link
Member

marcalff commented Apr 3, 2024

Nice work. Can we document this exporter as an section in README.md ?

Sorry for the missing. I will add documents tomorrow several days later. Sorry, I'm too busy recently.I will commit and push documents together with codes if there are more feedbacks about this PR.

Merging this PR now, please open a PR later for some doc when you have time.

Thanks for this work.

@marcalff marcalff changed the title OTLP file exporter. [EXPORTER] OTLP file exporter. Apr 3, 2024
@marcalff marcalff changed the title [EXPORTER] OTLP file exporter. [EXPORTER] OTLP file exporter Apr 3, 2024
@marcalff marcalff merged commit e86ceba into open-telemetry:main Apr 3, 2024
48 checks passed
@owent owent deleted the otlp_file_exporter branch April 26, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTLP file exporter
6 participants