[exporter/elasticsearch] Add internal telemetry for bulk indexer#41254
Conversation
| type bulkIndexerStats struct { | ||
| docsIndexed atomic.Int64 | ||
| } |
There was a problem hiding this comment.
[For reviewers] I removed the bulkIndexerStats since it was used in tests for assertions but the same thing can now be done by the metrics. Let me know if I missed something here.
| itemsCount := bi.Items() | ||
| if itemsCount == 0 { | ||
| return docappender.BulkIndexerResponseStat{}, nil | ||
| } |
There was a problem hiding this comment.
[For reviewers] Added this to short-circuit no items in the indexer. I couldn't think of any side-effects but let me know if I am missing something.
| case code == http.StatusTooManyRequests: | ||
| outcome = "too_many" | ||
| case code >= 500: | ||
| outcome = "failed_server" |
There was a problem hiding this comment.
[For reviewers] I was hoping mdatagen would generate some code/enums for the defined attributes but I don't think it does (or am I missing some config?). Anyway, these are used a couple of places so we can think of defining them as consts but I didn't think it necessary for now - let me know if you think otherwise.
| } | ||
|
|
||
| func (w *asyncBulkIndexerWorker) flush() { | ||
| // TODO (lahsivjar): Should use proper context else client metadata will not be accessible |
There was a problem hiding this comment.
[For reviewers] Will create a follow-up issue/PR for this
| level: alpha | ||
| enabled: true | ||
| description: Count of document retries. | ||
| extended_documentation: Only document level retries are captured, whole bulk request retries are not captured. |
There was a problem hiding this comment.
[For reviewers] This might come out as a bit surprising (since if the full request is 429 there are, by definition, docs in there that were retried). If I am reading the code right, we do something similar in go-docappender too (ref).
There was a problem hiding this comment.
request level retries are done by es client, not by docappender / es exporter. So I'm not too surprised.
| level: alpha | ||
| enabled: true | ||
| description: Count of document retries. | ||
| extended_documentation: Only document level retries are captured, whole bulk request retries are not captured. |
There was a problem hiding this comment.
request level retries are done by es client, not by docappender / es exporter. So I'm not too surprised.
|
We should be good to merge this one. PR's author is also a code owner, and it was approved by another one. |
edmocosta
added
the
ready to merge
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description PR #41254 introduced additional telemetry for ES exporter bulk indexers, however, it missed shuting down the telemetry builder. The changes are not released yet so I have not added any changelog. <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Related to #38610 <!--Describe what testing was performed and which tests were added.--> #### Testing N/A <!--Describe the documentation added.--> #### Documentation N/A <!--Please delete paragraphs that you did not use before submitting.-->
…n-telemetry#41254) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Adds additional telemetry to Elasticsearch exporter, mostly to observe the docappender. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#38610 <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests added <!--Describe the documentation added.--> #### Documentation Mdatagen docs updated <!--Please delete paragraphs that you did not use before submitting.-->
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description PR open-telemetry#41254 introduced additional telemetry for ES exporter bulk indexers, however, it missed shuting down the telemetry builder. The changes are not released yet so I have not added any changelog. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Related to open-telemetry#38610 <!--Describe what testing was performed and which tests were added.--> #### Testing N/A <!--Describe the documentation added.--> #### Documentation N/A <!--Please delete paragraphs that you did not use before submitting.-->
4 participants
Description
Adds additional telemetry to Elasticsearch exporter, mostly to observe the docappender.
Link to tracking issue
Fixes #38610
Testing
Unit tests added
Documentation
Mdatagen docs updated