-
Notifications
You must be signed in to change notification settings - Fork 3.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
[improve][io]add metrics to elastic search sink #20498
Closed
Closed
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
e886a89
[improve][sink]add metrics to elastic search sink
zzzming e3309d1
review comments
zzzming 3eeb7b5
separate two different ignore
zzzming cedaed0
bulk ops metrics
zzzming 90e90a1
move metrics to indexDocument
zzzming 2b29c53
review comments
zzzming e59871c
add missing metrics file
zzzming 81d8033
review feedback update
zzzming 3b95212
delete_attempt metrics
zzzming 11743cd
add incrementSuccessCounter
zzzming 381293c
fix success increments
zzzming 1f4329b
ci lint error
zzzming 616ad1a
add recordRecordIndexedSuccess() per review comment
zzzming File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 guess the only concern about the metrics is that when bulk indexing is used, the result is probably asynchronous. @nicoloboschi is this the case?
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.
Fantastic point. Note also that
indexDocument
can fail without throwing an exception:pulsar/pulsar-io/elastic-search/src/main/java/org/apache/pulsar/io/elasticsearch/ElasticSearchClient.java
Lines 184 to 190 in 82237d3
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.
Perhaps we can integrate with this interface?
pulsar/pulsar-functions/api-java/src/main/java/org/apache/pulsar/functions/api/Record.java
Lines 106 to 116 in 82237d3
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.
It almost seems like we could get a generic metrics collector for acks on all sinks.
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.
We don't need the generic solution in this PR, of course, but we do need to handle the async behavior of writing to ES.
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.
@michaeljmarshall @lhotari
Added metrics to bulk request case