-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add AlibabaCloud LogService exporter #259
Conversation
Codecov Report
@@ Coverage Diff @@
## master #259 +/- ##
==========================================
+ Coverage 79.64% 80.90% +1.25%
==========================================
Files 163 169 +6
Lines 8338 8887 +549
==========================================
+ Hits 6641 7190 +549
Misses 1343 1343
Partials 354 354
Continue to review full report at Codecov.
|
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.
On shutdown exporters should flush any internal queues to ensure all metrics/traces are sent before returning. Does the producer you're using have those and is there a way of waiting for them to finish if so?
- `max_retry` (optional): max retry count when send fail. | ||
- `max_buffer_size` (optional): max buffer used in memory. |
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.
@shabicheng Apologies for the delayed review.
Retries and buffers are provided by processors. This ensures that it's handled consistently and that limits are applied globally. See these docs:
https://github.com/open-telemetry/opentelemetry-collector/blob/master/processor/queuedprocessor
https://github.com/open-telemetry/opentelemetry-collector/blob/master/processor/memorylimiter
https://github.com/open-telemetry/opentelemetry-collector/blob/master/processor/batchprocessor
@bogdandrutu please correct me if wrong.
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.
Sending data to LogService with the producer is best practice, the producer will use different retry strategy according to the error code from the server. The max retry count and max buffer size are very important for the producer, so we expose these params.
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.
@tigrannajaryan what are your thoughts? Is it a showstopper if retries and buffers are not being managed by the processors? I assume so given it defeats the purpose of having those things centrally managed but just verifying.
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.
@jrcamp To avoid conceptual conflicts, I have removed the producer and use a normal client instead. And we will use some processors to do retry/batch/memory limit. If we have more requirements for processors, I will submit some processor PRs.
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.
Great! One thing to keep in mind is that if you want to send things in parallel you'll still have to implement that part yourself. grep for NumWorkers to see current examples of it.
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.
Actually you may can just use https://github.com/open-telemetry/opentelemetry-collector/tree/master/processor/queuedprocessor. Can you see if that will work for your use cases?
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.
@jrcamp Thanks for reminding us, we can use queuedprocessor directly. Actually we will use the combination of memorylimiter&batchprocessor&queuedprocessor.
Also please rebase since things have been delayed. |
Thanks for reminding, the producer can gracefully close and I've added shutdown function for the exporter. |
The load test is failed, but it doesn't seem to be related to this PR |
lgtm, I think last thing to address is comment above about if you need to handle parallelism here or not. |
Now the target code coverage is 95.00%, we will add more tests to achieve the threshold. |
@shabicheng is this ready for merge? |
@shabicheng please run |
* Add alibabacloud_logservice exporter * refine code for ci * refine code for review commends * upgrade latest producer version * trgger ci * remove producer, use raw logservice client * refine code and add more unittest
* INITIAL * Properly implement name * reword comment * address comments and rename stop to shutdown * Use assert in tests, change to shutdown instead of shutdownFunc and add some missing comments.
* export noop meter * move mock meter to internal/metric package
Description:
This PR introduces an exporter for AlibabaCloud LogService. The exporter works by translating spans and metrics into the LogService protobuf format expected by LogService, and sending over HTTP.
The trace model is similar to Jaeger on Aliyun Log Service. The metrics model in LogService can be used as Prometheus's remote adapter.
Testing:
Unit tests added for translating resources, spans, and metrics to the LogService model. Now code coverage is 86.5%.
Manually tested, sending to an LogService's instance.
Documentation:
Added a README, which describes the exporter's config.