[Maps] Remove maps-telemetry saved object as it is no longer in use by kindsun · Pull Request #69871 · elastic/kibana

kindsun · 2020-06-25T01:03:07Z

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 · 2020-06-25T01:03:09Z

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 · 2020-06-25T09:12:45Z

x-pack/test/api_integration/apis/telemetry/telemetry_local.js

+      expect(stats.stack_stats.kibana.plugins['maps-telemetry'].timeCaptured).to.be.a('string');
+      expect(stats.stack_stats.kibana.plugins['maps-telemetry'].attributes).to.be(undefined);
+      expect(stats.stack_stats.kibana.plugins['maps-telemetry'].id).to.be(undefined);
+      expect(stats.stack_stats.kibana.plugins['maps-telemetry'].type).to.be(undefined);


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',.

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.

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 :)

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

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

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

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 });

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

afharo · 2020-06-25T09:32:04Z

x-pack/plugins/maps/server/saved_objects/maps_telemetry.ts

-          layerTypesCount: { dynamic: 'true', properties: {} },
-          emsVectorLayersCount: { dynamic: 'true', properties: {} },


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)

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.

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?

@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!

…or SO. Update tests and remove unused constant

…ut set as type object

afharo · 2020-06-25T11:54:49Z

x-pack/plugins/maps/common/constants.ts

@@ -25,7 +25,6 @@ export const EMS_TILES_VECTOR_TILE_PATH = 'vector/tile';
 export const MAP_SAVED_OBJECT_TYPE = 'map';


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 :)

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

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 · 2020-06-25T16:09:06Z

💚 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. 🤞

… use (#69871) (#69980)

… use (#69871) (#69983)

maps -> maps-telemetry & dynamic fields removed

11490df

kindsun added Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:Telemetry v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v7.8.1 labels Jun 25, 2020

kindsun requested a review from a team as a code owner June 25, 2020 01:03

kindsun mentioned this pull request Jun 25, 2020

Explosion of mapping fields exceeds default 1k limit on upgrade #69869

Closed

tylersmalley mentioned this pull request Jun 25, 2020

[SECURITY] Updates mappings and re-enables tests #69872

Closed

kindsun mentioned this pull request Jun 25, 2020

[Telemetry] Collector Schema #64942

Merged

afharo requested changes Jun 25, 2020

View reviewed changes

Aaron Caldwell added 2 commits June 25, 2020 05:25

Requested changes. Use 'maps' for collection type, 'maps-telemetry' f…

e2fbf08

…or SO. Update tests and remove unused constant

Retain saved object fields layerTypesCount and emsVectorLayersCount b…

f798091

…ut set as type object

kindsun requested a review from afharo June 25, 2020 11:48

afharo reviewed Jun 25, 2020

View reviewed changes

Remove old telemetry/task mgr saved object and refs

c02e374

afharo approved these changes Jun 25, 2020

View reviewed changes

kindsun changed the title ~~[Maps] Rollback change to maps telemetry saved object name and remove dynamic fields~~ [Maps] Remove maps-telemetry saved object as it is no longer in use Jun 25, 2020

thomasneirynck approved these changes Jun 25, 2020

View reviewed changes

kindsun merged commit 40c746e into elastic:master Jun 25, 2020

kindsun deleted the rollback-telemetry-saved-object-change-and-remove-dynamic-fields branch June 25, 2020 16:38

kindsun pushed a commit that referenced this pull request Jun 25, 2020

[7.x] [Maps] Remove maps-telemetry saved object as it is no longer in…

e4ddfb8

… use (#69871) (#69980)

kindsun pushed a commit that referenced this pull request Jun 25, 2020

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

5144f99

… use (#69871) (#69983)

kindsun mentioned this pull request Jun 30, 2020

[Maps] Telemetry fixes & updates #70334

Closed

6 tasks

		layerTypesCount: { dynamic: 'true', properties: {} },
		emsVectorLayersCount: { dynamic: 'true', properties: {} },

		@@ -25,7 +25,6 @@ export const EMS_TILES_VECTOR_TILE_PATH = 'vector/tile';
		export const MAP_SAVED_OBJECT_TYPE = 'map';

Conversation

kindsun commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 25, 2020

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afharo Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kindsun Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kindsun Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afharo left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 25, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

thomasneirynck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kindsun commented Jun 25, 2020 •

edited

Loading

afharo Jun 25, 2020 •

edited

Loading

kindsun Jun 25, 2020 •

edited

Loading

kindsun Jun 25, 2020 •

edited

Loading

thomasneirynck left a comment •

edited

Loading