-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 metrics otelcol_exporter_queue_capacity #5475
add metrics otelcol_exporter_queue_capacity #5475
Conversation
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@bogdandrutu Could you help remove this stale label or review this pr? I saw that @jpkrohling is on leave now. |
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.
The change looks good, please add a test to TestQueuedRetry_QueueMetricsReported
to ensure the metric is reported
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.
Please also add an entry to the changelog noting the new metric
d6aaaf0
to
c2b5f0f
Compare
Codecov Report
@@ Coverage Diff @@
## main #5475 +/- ##
==========================================
- Coverage 91.53% 91.48% -0.06%
==========================================
Files 192 192
Lines 11398 11410 +12
==========================================
+ Hits 10433 10438 +5
- Misses 770 775 +5
- Partials 195 197 +2
Continue to review full report at Codecov.
|
Done |
@codeboten Done |
@codeboten It's really weird, the report said the TestQueuedRetry_QueueMetricsReported failed, but I run the test by myself using exactly same branch and in mac and linux, they both passed. Do you know what kind of reason may cause this, or what is the difference between the CI and local development? |
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.
Left a small nit but otherwise LGTM.
exporter/exporterhelper/obsreport.go
Outdated
@@ -55,6 +56,12 @@ func newInstruments(registry *metric.Registry) *instruments { | |||
metric.WithLabelKeys(obsmetrics.ExporterKey), | |||
metric.WithUnit(metricdata.UnitDimensionless)) | |||
|
|||
insts.queueCapacity, _ = registry.AddInt64DerivedGauge( | |||
obsmetrics.ExporterKey+"/queue_capacity", | |||
metric.WithDescription("Current capacity of the retry queue (in batches)"), |
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.
I feel like "Current" implies the capacity can change at some point, but until the collector is restarted it is a fixed size right? Maybe something like "Fixed capacity..." is clearer?
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.
You are right, I will fix this
@fatsheep9146 the latest runs show TestQueuedRetry_QueueMetricsReported passing. Do you see the same? |
aec2071
to
9ff9377
Compare
No I didn't see the same result. The result I saw is as following
Yet I run test TestQueuedRetry_QueueMetricsReported in local, the test case passed. I'm really confused. |
You are correct, I see the failure you are seeing. I'm not sure why it happens. |
b00150a
to
30b28de
Compare
@TylerHelmuth |
@codeboten @jpkrohling ping for review this pr again, thanks :-) |
@fatsheep9146 @codeboten does the |
Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
30b28de
to
e8353e1
Compare
@TylerHelmuth @jpkrohling The monitoring doc are also updated. Please help review this again =D |
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.
2 small nits in the new documentation, otherwise LGTM
Co-authored-by: Tyler Helmuth <[email protected]>
Co-authored-by: Tyler Helmuth <[email protected]>
Signed-off-by: Ziqi Zhao [email protected]
Description:
Fix #4902, add metric otelcol_exporter_queue_capacity
Link to tracking Issue:
#4902
Testing:
Documentation: