Skip to content
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

[Monitoring] Rename ccr fields based on changes in ES #24519

Merged

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Oct 24, 2018

Relates to elastic/elasticsearch#34703 and #23013

NOTE: This PR will fail CI and not work until the appropriate ES PR is merged. elastic/elasticsearch#34836

This PR updates the UI code to account for field names changing in ES. The changed fields are located here: elastic/elasticsearch#34703 (comment)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@bleskes
Copy link
Contributor

bleskes commented Oct 25, 2018

Here's the relevant PR on the ES side: elastic/elasticsearch#34836

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chrisronline . I left some comments. I also wanted to double check that the ui change to accomodate the move from a leader cluster to a remote cluster?

@@ -32,7 +32,7 @@ async function getCcrStat(req, esIndexPattern, filters) {
filterPath: [
'hits.hits._source.ccr_stats',
'hits.hits._source.timestamp',
'hits.hits.inner_hits.oldest.hits.hits._source.ccr_stats.number_of_operations_indexed',
'hits.hits.inner_hits.oldest.hits.hits._source.ccr_stats.operations_read',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be operations_written.

@@ -32,7 +32,7 @@ async function getCcrStat(req, esIndexPattern, filters) {
filterPath: [
'hits.hits._source.ccr_stats',
'hits.hits._source.timestamp',
'hits.hits.inner_hits.oldest.hits.hits._source.ccr_stats.number_of_operations_indexed',
'hits.hits.inner_hits.oldest.hits.hits._source.ccr_stats.operations_read',
'hits.hits.inner_hits.oldest.hits.hits._source.ccr_stats.number_of_failed_fetches',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one should be failed_read_requests

},
"timestamp": "2018-09-19T20:01:00.440Z",
"oldestStat": {
"number_of_failed_fetches": 0,
"number_of_operations_indexed": 1
"operations_read": 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operations_written

"number_of_operations_indexed": 86,
"fetch_exceptions": [],
"time_since_last_fetch_millis": 19886
"operations_read": 86,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operations_written

@chrisronline
Copy link
Contributor Author

Update here.

I've verified that all tests pass locally but they will most likely continue to fail on CI until the snapshot the CI tool uses for ES is updated to reflect the latest changes on the ES side (@ruflin kicked off the build so it should be soon-ish).

I propose we skip the tests that are failing in this PR so we can get green CI, then merge it in. I'll then open another PR where these tests are re-added and we merge that once CI is using the right snapshot and the tests are passing in the CI environment.

Here are the tests passing locally with the latest ES build:

Elastic-MBP:x-pack chris$ node ../scripts/functional_test_runner.js --config test/api_integration/config.js --grep "ccr"
 info Config loaded
 info Starting tests

 └-: apis
   └-> "before all" hook
   └-: Monitoring
     └-> "before all" hook
     └-: Elasticsearch
       └-> "before all" hook
       └-: ccr
         └-> "before all" hook
         └-> "before all" hook: load archive
           │ info [monitoring/ccr] Loading "mappings.json"
           │ info [monitoring/ccr] Loading "data.json.gz"
           │ info [monitoring/ccr] Created index ".monitoring-es-6-2018.09.19"
           │ info [monitoring/ccr] Indexed 3335 docs into ".monitoring-es-6-2018.09.19"
         └-> should return all followers and a grouping of stats by follower index
           └-> "before each" hook: global before each
           └- ✓ pass 
         └-> "after all" hook: unload archive
           │ info [monitoring/ccr] Unloading indices from "mappings.json"
           │ info [monitoring/ccr] Deleted existing index ".monitoring-es-6-2018.09.19"
           │ info [monitoring/ccr] Unloading indices from "data.json.gz"
         └-> "after all" hook
       └-: ccr shard
         └-> "before all" hook
         └-> "before all" hook: load archive
           │ info [monitoring/ccr] Loading "mappings.json"
           │ info [monitoring/ccr] Loading "data.json.gz"
           │ info [monitoring/ccr] Created index ".monitoring-es-6-2018.09.19"
           │ info [monitoring/ccr] Indexed 3335 docs into ".monitoring-es-6-2018.09.19"
         └-> should return specific shard details
           └-> "before each" hook: global before each
           └- ✓ pass 
         └-> "after all" hook: unload archive
           │ info [monitoring/ccr] Unloading indices from "mappings.json"
           │ info [monitoring/ccr] Deleted existing index ".monitoring-es-6-2018.09.19"
           │ info [monitoring/ccr] Unloading indices from "data.json.gz"
         └-> "after all" hook
       └-> "after all" hook
     └-: common
       └-> "before all" hook
       └-: mappings
         └-> "before all" hook
         └-> "before all" hook: load mappings
         └-> "before all" hook: load mappings
         └-> "before all" hook: load mappings
         └-> "before all" hook: load mappings
         └-> "before all" hook: load mappings
         └-: for es metrics
           └-> "before all" hook
           └-> ccr_stats.time_since_last_read_millis should exist in the mappings
             └-> "before each" hook: global before each
             └- ✓ pass 
           └-> ccr_stats.leader_max_seq_no should exist in the mappings
             └-> "before each" hook: global before each
             └- ✓ pass 
           └-> ccr_stats.follower_global_checkpoint should exist in the mappings
             └-> "before each" hook: global before each
             └- ✓ pass 
           └-> "after all" hook
         └-> "after all" hook
       └-> "after all" hook
     └-> "after all" hook
   └-> "after all" hook


5 passing (4.0s)

@chrisronline
Copy link
Contributor Author

I talked with @bleskes @ruflin and @ycombinator about merging this PR before waiting for CI and we're in favor of it.

Pinging @epixa for thoughts

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline merged commit af3e621 into elastic:master Oct 25, 2018
@chrisronline chrisronline deleted the monitoring/ccr_rename_fields branch October 25, 2018 16:31
chrisronline added a commit to chrisronline/kibana that referenced this pull request Oct 25, 2018
* Rename ccr fields based on changes in ES

* More renames

* Update archive data

* Update snapshot

* Skip the api integration tests for now
chrisronline added a commit that referenced this pull request Oct 25, 2018
* Rename ccr fields based on changes in ES

* More renames

* Update archive data

* Update snapshot

* Skip the api integration tests for now
@chrisronline
Copy link
Contributor Author

Backport:

6.x: 2182e27

@chrisronline
Copy link
Contributor Author

PR to re-enable tests: #24600

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants