-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Expose external refreshes through the stats API #38643
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
Conversation
|
Pinging @elastic/es-distributed |
s1monw
left a comment
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.
LGTM left some nit-picks. I am sorry for the delay on this. This looks pretty good, thank you!
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.
extra newline
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.
extra newline
|
@elasticmachine ok to test |
|
@clandry94 can you merge your branch up to current master please |
7e4f177 to
82695e9
Compare
|
@clandry94 there are some test failures, would you mind taking a look? |
|
Sure, I'll resolve them |
82695e9 to
f70f6cc
Compare
|
@clandry94 you don't have to forcepush for all changes it's easier to review if you don't. you can still just do a |
|
@s1monw in the elasticsearch-ci/2 The actual values seem to be consistent over multiple tests, so should the expected values be changed to match that? |
|
Sorry for the late reply.
I think this change is the reason for this off-by-one: https://github.com/elastic/elasticsearch/pull/38643/files#diff-07c9dc27cfd5ff0ef6a21088ddf8b8d6R327 |
|
you also need to merge with master since we updated to a new and incompatible lucene snapshot. |
1fb6074 to
95f1751
Compare
|
@clandry94 I tried to get this ready, but I did not have your permission. Could you please merge the master branch to your branch and apply this patch 38643.txt? We need to handle BWC. Thank you! |
|
Hey @dnhatn, merged master and applied your patch. Thanks for the help 😁 |
|
run elasticsearch-ci/packaging-sample |
|
Hey @clandry94, can you please merge master into your branch? BWC was disabled in the revision that you merged. Thank you! |
|
Thanks @clandry94 for working on this. |
Having these new fields to the basic cat shards yaml test does not add much while they are causing a BWC issue (i.e., we need to skip this yaml test until 8.0). Relates #38643
BWC is failing with this change. This reverts commit 4d73485.
Right now, the stats API only provides refresh metrics regarding internal refreshes. This isn't very useful and somewhat misleading for cluster administrators since the internal refreshes are not indicative of documents being available for search. In this PR I added a new metric for collecting external refreshes as they occur and exposing them through the stats API. Now, calling an endpoint for stats will yield external refresh metrics as well. Relates elastic#36712
Right now, the stats API only provides refresh metrics regarding internal refreshes. This isn't very useful and somewhat misleading for cluster administrators since the internal refreshes are not indicative of documents being available for search. In this PR I added a new metric for collecting external refreshes as they occur and exposing them through the stats API. Now, calling an endpoint for stats will yield external refresh metrics as well. Relates #36712
Right now, the stats API only provides refresh metrics regarding internal refreshes. This isn't very useful and somewhat misleading for cluster administrators since the internal refreshes are not indicative of documents being available for search. In this PR I added a new metric for collecting external refreshes as they occur and exposing them through the stats API. Now, calling an endpoint for stats will yield external refresh metrics as well. Relates #36712
An implementation from this issue #36712.
Right now, the stats API only provides refresh metrics regarding internal refreshes. This isn't very useful and somewhat misleading for cluster administrators since internal refreshes are not indicative of documents being available for search..
In this PR I added a new metric for collecting external refreshes as they occur and exposing them through the stats API. Now, calling an endpoint for stats will yield external refresh metrics as well.
Also, how is the naming? I'm not sure
external_totalandexternal_total_time_in_millisis the best way to express this, but it gets the point across. Also, shouldtotalandtotal_time_in_millisbe changed tointernal_totaland so on?cc @s1monw