Allow kibana_settings collector to return nothing #22091
Conversation
|
@chrisronline does this impact anything with #21100 ? |
| }; | ||
| // return nothing when there was no result | ||
| let settingsDoc; | ||
| if (kibanaSettingsData != null) { // test null or undefined |
There was a problem hiding this comment.
This won't catch undefined, but reading through the code it doesn't have to do that.
There was a problem hiding this comment.
++ I'd just update the comment to not say or undefined.
There was a problem hiding this comment.
Using double-equals to check for null or undefined is a habit I've acquired from working in Canvas:
> var foo
> foo == null ? "hi" : "bye"
"hi"But you are right, the code doesn't have to do that. The comment is accurate though.
There was a problem hiding this comment.
Oh that's interesting and subtle. Not a fan :), but I suppose it works.
There was a problem hiding this comment.
pushed b476642 to remove any unclarity
|
I added a note in the description about why this PR is not a blocker fix. |
|
@pickypg what do you think about just closing this PR? It doesn't fix a blocker. A big concern Shaunak and I found is that this fix will not work in the long term: keeping track of state ( |
💔 Build Failed |
|
Personally I think changing it (by closing this PR) will cause a breaking change and also leave us with an unexpected / broken state in the future since we'd be intentionally leaving in a bug. |
|
jenkins test this (Kibana failed to start in one test) |
I don't understand this concern? Can you explain this more please? |
Yes, potentially. I'll address this in a follow up PR Here: #22107 |
💔 Build Failed |
Sorry, I was mistaken about that concern. There is an issue with the kibana_settings API: it should always return the same shape of data, whether In my mistaken understanding, the change in this PR would be a temporary stop-gap. But @pickypg is right that this change is going to stop a breaking change from happening. There's always been this optimization to limit how many documents are indexed, via the statefulness of I'm putting blocker label back on this PR. |
jenkins test this |
+1 |
Is it fair to say this hasn't been the actual case though? Currently in master, we always return some structure so theoretically something is always indexed for each internal collector round trip? Or did you filter it out in the bulk uploader? |
It is intended to be filtered out in the bulk uploader, via this truthy check: https://github.com/elastic/kibana/blob/master/x-pack/plugins/monitoring/server/kibana_monitoring/bulk_uploader.js#L126 That truthy check was working in 6.3.x because the settings data result wasn't automatically decorated with the "kibana" object that comes from Now, in master, the settings data is decorated with that kibana property to simplify the combining of different types into documents for bulk upload. |
💔 Build Failed |
|
jenkins test this |
💔 Build Failed |
|
I'm consistently getting this test failure that has been filed as a flaky test: #21664 |
* Fix kibana_settings collector to return nothing when no settings data is found * make code more clear
* Fix kibana_settings collector to return nothing when no settings data is found * make code more clear
This fixes a bug that was causing a NPE with a Painless script inside a watch in Elasticsearch. It was found along with elastic/elasticsearch#32923
Important
This PR restores the kibana_settings data to the original behavior in 6.3.x, which was to NOT index multiple documents when an admin email is not set in the kibana settings. It doesn't fix a problem that users would see, but it does resolve a breaking, unexpected change in behavior for the data getting indexed.