opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

OTLP file exporter.

Open owent opened this issue 1 year ago • 3 comments

Fixes #2482

Changes

Please provide a brief description of the changes here.

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

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

owent avatar Feb 18 '24 09:02 owent

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

owent avatar Feb 19 '24 12:02 owent

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

perhapsmaple avatar Feb 19 '24 14:02 perhapsmaple

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.

owent avatar Feb 20 '24 02:02 owent

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

marcalff avatar Mar 21 '24 11:03 marcalff

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

Done

owent avatar Mar 25 '24 11:03 owent

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

lalitb avatar Mar 29 '24 18:03 lalitb

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.

owent avatar Apr 02 '24 14:04 owent

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 avatar Apr 03 '24 17:04 marcalff