[Maps] Remove maps-telemetry saved object as it is no longer in use

[kindsun]

Resolves #69869. This PR removes the maps-telemetry saved object. This is no longer required and appears to be left over from older logic when maps leveraged task manager. I've confirmed this saved object is not used by either the Maps or Pulse teams and has no effect on reported telemetry. i.e.- The stats endpoint returns the correct values and both the local jest tests and telemetry functional tests are passing.

This should resolve both the dynamic mappings and name change issues since the saved object that was causing these issues no longer exists.

The shape of the telemetry will be unaffected by this PR. Here's a snapshot of the maps telemetry with current changes in place as reported at http://localhost:5601/<dev env prefix>/api/stats?extended=true:

"maps":{
  "settings":{
    "show_map_visualization_types":false
  },
  "index_patterns_with_geo_field_count":2,
  "index_patterns_with_geo_point_field_count":2,
  "index_patterns_with_geo_shape_field_count":0,
  "maps_total_count":2,
  "time_captured":"2020-06-25T15:06:50.393Z",
  "attributes_per_map":{
    "data_sources_count":{
      "min":4,
      "max":4,
      "avg":4
    },
    "layers_count":{
      "min":4,
      "max":4,
      "avg":4
    },
    "layer_types_count":{
      "vector_tile":{
        "min":1,
        "max":1,
        "avg":1
      },
      "vector":{
        "min":3,
        "max":3,
        "avg":3
      }
    },
    "ems_vector_layers_count":{
      "world_countries":{
        "min":1,
        "max":1,
        "avg":0.5
      }
    }
  }
},

cc. @afharo

[elasticmachine]

Pinging @elastic/kibana-gis (Team:Geo)

[afharo]

I think there's a bit of confusion going on here:
The SavedObjects type and its mappings are not related to the Usage Collector type.
Changing the type of the usageCollector: https://github.com/aaronjcaldwell/kibana/blob/rollback-telemetry-saved-object-change-and-remove-dynamic-fields/x-pack/plugins/maps/server/maps_telemetry/collectors/register.ts#L22

Does not affect the actual saved object type (the one leading to mapping field explosion): https://github.com/aaronjcaldwell/kibana/blob/rollback-telemetry-saved-object-change-and-remove-dynamic-fields/x-pack/plugins/maps/server/saved_objects/maps_telemetry.ts#L9

Another proof of it, it's the getMapSavedObjects method is using the constant MAP_SAVED_OBJECT_TYPE, which happens to be maps.

maps-telemetry SO type is no longer defined in the mappings after #69816. We may want to revert it to avoid having maps and maps-telemetry SOs in long-running clusters. But we'll need to add a migration script if we are removing the keys layerTypesCount and emsVectorLayersCount (otherwise the migration will likely fail to store existing documents), unless we use the type: 'object' approach.

@joshdover what do you think?

[afharo]

I sincerely don't think the savedObjects type should affect the name of the reported telemetry.
How we obtain the telemetry to be returned in the fetch method in the usageCollector (in this case, reading the maps-telemetry SO) should not affect the name of the reported telemetry maps:
Simply change the line type: TELEMETRY_TYPE, to type: 'maps',.

[kindsun]

I've changed it to have the saved object as maps_telemetry and the usageCollector type as maps as you've described.

Another proof of it, it's the getMapSavedObjects method is using the constant MAP_SAVED_OBJECT_TYPE, which happens to be maps

I don't quite follow you on this one. MAP_SAVED_OBJECT_TYPE is actually map singular, so not looking at the same thing. This is likely why we named maps_telemetry as it was named in the first place, to not have maps and map SOs.

[afharo]

I think you are right MAP_SAVED_OBJECT_TYPE is not related at all. Maybe we are not storing the maps-telemetry SavedObjects at all anymore so the mappings are simply not needed?

I'll clone this branch after lunch and I'll give it a go to fully understand what SavedObjects are for what :)

[kindsun]

As a follow-up question, how is telemetry keeping track of the maps saved object type maps-telemetry? On our end this feels a little disconnected. We're registering a collector with type maps that collects data conforming to the shape of the saved object maps-telemetry that we define in our mappings. It's fine to have the names not be 1:1, but the connection between the two isn't clear to me

[kindsun]

Our messages crossed paths but I think we're more or less saying the same thing. Maybe this just needs to be removed then

[kindsun]

I thought previously this might've been used for some old task manager logic, but looking back at that it shouldn't have needed a saved object there either since really we were just grabbing telemetry 1x/24 hours and writing it to task manager state, no SO required so far as I can tell. When we first implemented maps telemetry, it was mostly just copy and paste from existing sources and then updates regarding the shape of our data. It looks like I may have just stored an extra saved object along the way that should be removed, my mistake. 🤢

If it's not being used on your end, which it sounds like it's not, I say we just remove it

[afharo]

Yeah! I think it evolved from time to time: Initially, the task manager logic was generating the saved object and the fetch method was simply returning it.
Then the task manager logic was removed but we kept the saved object in place.
But, to make it clear: the usageCollection/telemetry plugin does not rely on any savedObjects created by the registered usageCollectors. It simply calls the fetch method and appends the returned payload into the big telemetry payload under stack_stats.kibana.plugins[type].
In pseudocode:

for (const collector of registeredUsageCollectors) {
  fullTelemetryPayload.stack_stats.kibana.plugins[collector.type] = await collector.fetch(callCluster);
}

await http.post(TELEMETRY_URL, { body: fullTelemetryPayload });

[afharo]

From the telemetry POV, I'd say it's safe to remove that mappings definition :)

[kindsun]

Removed!

[afharo]

Would it be enough if we did layerTypesCount: { type: 'object' }? That would allow us storing the object but not indexing each value in it (since we are not actually searching/aggregating for those values)

[kindsun]

That might make more sense here as a placeholder since we do want to retain some version of these moving forward but without using dynamic fields. I've added back in the logic supporting construction of these fields and changed their types per your recommendation.

[rudolf]

we also need to set enabled: false to prevent these from being indexed. If we're not querying this telemetry data from Kibana, can we just set the entire maps-telemetry mapping to type: 'object', enabled: false?

[kindsun]

@rudolf Per the conversation above, I've actually removed this saved object. It appears to be leftover from older logic and is no longer required!

[afharo]

I think this constant needs to match the name in x-pack/plugins/maps/server/saved_objects/maps_telemetry.ts: maps-telemetry. Because we are searching for it in the getMapsTelemetry method :)

[afharo]

Hold on! Scratch that! Is this related to some other saved objects maps is maintaining?

[kindsun]

Yeah, looks like you figured this out in the above conversation but this is unrelated. This defines the constant for our saved maps of type map.

[afharo]

LGTM

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: c02e374

Build metrics

✅ unchanged

History

• 💚 Build #56249 succeeded f798091
• 💚 Build #56153 succeeded 11490df

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[thomasneirynck]

code review lgtm. thx. let's see how this removal behaves on 7.8. 🤞