[APM] Use SavedObjectsClient from the request context for APM indices endpoints #49994

ogupte · 2019-11-02T07:19:16Z

Updated APM Indices endpoints to use the SavedObjectsClient from the legacy request context, and set the apm-indices schema object to be namspace-agnostic.

elasticmachine · 2019-11-02T08:41:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6ccb72c020910bbd420ff3c173ecf860e544e7b7

sorenlouv · 2019-11-03T21:40:50Z

Thanks for looking into this! I'll take a better look after EAH but overall looks good 👍

sorenlouv · 2019-11-06T04:41:55Z

x-pack/legacy/plugins/apm/server/lib/apm_telemetry/index.ts

+  };
+}
+
+export async function storeApmTelemetry(


Can you rename this, so it's clear that it's just telemetry for the service overview page and not apm telemetry in general? Something like storeApmServicesTelemetry

sorenlouv · 2019-11-06T05:29:25Z

x-pack/legacy/plugins/apm/server/lib/apm_telemetry/index.ts

+) {
+  try {
+    const savedObjectsClient = getSavedObjectsClient(server);
+    await savedObjectsClient.create('apm-telemetry', apmTelemetry, {


Shouldn't this use the constant APM_TELEMETRY_DOC_ID?

I didn't do this because it would conflate the saved object type (the string literal) with the saved object id (the constant). I could alternatively make a second constant APM_TELEMETRY_SAVED_OBJ_TYPE or rename APM_TELEMETRY_DOC_ID to APM_TELEMETRY_DOC_ID_AND_SAVED_OBJ_TYPE and use it in both places.

I think having two constants makes sense. You did something similar of ui indices:

kibana/x-pack/legacy/plugins/apm/server/lib/settings/apm_indices/get_apm_indices.ts

Lines 26 to 27 in 1cc2a34

export const APM_INDICES_SAVED_OBJECT_TYPE = 'apm-indices';

export const APM_INDICES_SAVED_OBJECT_ID = 'apm-indices';

I'm wondering if it's better to have a apm_saved_object_constants.ts file where all saved objects are defined?

sorenlouv · 2019-11-06T05:29:34Z

x-pack/legacy/plugins/apm/server/lib/apm_telemetry/index.ts

+      const savedObjectsClient = getSavedObjectsClient(server);
+      try {
+        const apmTelemetrySavedObject = await savedObjectsClient.get(
+          'apm-telemetry',


constant APM_TELEMETRY_DOC_ID?

see https://github.com/elastic/kibana/pull/49994/files#r344903051

sorenlouv · 2019-11-06T05:30:27Z

x-pack/legacy/plugins/apm/server/lib/apm_telemetry/index.ts

+import { InternalCoreSetup } from 'src/core/server';
+import { isAgentName } from '../../../common/agent_name';
+import { getSavedObjectsClient } from '../helpers/saved_objects_client';
+export const APM_TELEMETRY_DOC_ID = 'apm-telemetry';


Is it possible to rename this to apm-services-telemetry to indicate it is not all apm telemetry data?

Yes we can do this, but it might be better to do this in the next minor version since we're so close to release, and I don't want to alarm folks by triggering a re-mapping of apm saved-objects.

since we're so close to release, and I don't want to alarm folks by triggering a re-mapping of apm saved-objects

I think I said 7.5 as well at some point but APM UI Indices is in 7.6, not 7.5, right? So this should go into 7.x not 7.5.

This PR is targeted for 7.5, so I'm hesitant to make that change here, but sure i can do it in a another PR targeted for 7.x.

x-pack/legacy/plugins/apm/server/lib/helpers/es_client.ts

sorenlouv · 2019-11-11T10:50:13Z

x-pack/legacy/plugins/apm/server/lib/settings/agent_configuration/create_agent_config_index.ts


 export async function createApmAgentConfigurationIndex(
  core: InternalCoreSetup
 ) {
  try {
    const { server } = core.http;
-    const indices = await getApmIndices(server);
+    const config = server.config();
+    const savedObjectsClient = getSavedObjectsClient(server);


What do you think about renaming this to:

Suggested change

const savedObjectsClient = getSavedObjectsClient(server);

const unscopedSavedObjectsClient = getUnscopedSavedObjectsClient(server);

Or perhaps:

Suggested change

const savedObjectsClient = getSavedObjectsClient(server);

const internalSavedObjectsClient = getInternalSavedObjectsClient(server);

Just to make the distinction between the "scoped" (getScopedSavedObjectsClient) and unscoped saved objects client really obvious.

Btw. just like you can do server.savedObjects.getScopedSavedObjectsClient, is it also possible to do something like: server.savedObjects.getUnScopedSavedObjectsClient so we don't have to init our own?

I like the idea of making this more obvious, of the two options internalSavedObjectsClient would be more consistent with how we refer to it in other places.

Thanks for chiming in @rudolf! We'll stick to internalSavedObjectsClient then.
Btw. do you know if an instance of internalSavedObjectsClient already exists or do we need to instantiate our own?

you need to create your own like here, what APM is doing isn't wrong, but is unusual for Kibana plugins so there's no convenience factory for that. In the NP we will probably expose an internalRepository but not a internalSavedObjectsClient (they're almost the same, the repository just has a few more methods). The idea is to make it clear that you're doing something unusual and unless you have a special reason like a legacy architecture that doesn't support spaces or writing/reading telemetry data, you probably should use the scoped saved objects client.

x-pack/legacy/plugins/apm/server/lib/settings/agent_configuration/create_agent_config_index.ts

elasticmachine · 2019-11-12T01:44:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b3a4c0821323e3114aca5093c4ada035e73e4ad9

elasticmachine · 2019-11-12T20:59:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 561445ceb2fb19c76ef1964810215d9344290379

sorenlouv · 2019-11-12T21:33:18Z

x-pack/legacy/plugins/apm/common/apm_saved_object_constants.ts

+  'apm-services-telemetry';
+export const APM_SERVICES_TELEMETRY_DOC_ID = 'apm-services-telemetry';
+export const APM_INDICES_SAVED_OBJECT_TYPE = 'apm-indices';
+export const APM_INDICES_DOC_ID = 'apm-indices';


WDYT about grouping them, and making the "id" constant naming consistent with the "type"?

// APM Services telemetry export const APM_SERVICES_TELEMETRY_SAVED_OBJECT_TYPE = 'apm-services-telemetry'; export const APM_SERVICES_TELEMETRY_SAVED_OBJECT_ID = 'apm-services-telemetry'; // APM indices export const APM_INDICES_SAVED_OBJECT_TYPE = 'apm-indices'; export const APM_INDICES_SAVED_OBJECT_ID = 'apm-indices';

sorenlouv · 2019-11-12T21:34:01Z

x-pack/legacy/plugins/apm/index.ts

@@ -48,7 +48,10 @@ export const apm: LegacyPluginInitializer = kibana => {
      },
      hacks: ['plugins/apm/hacks/toggle_app_link_in_nav'],
      savedObjectSchemas: {
-        'apm-telemetry': {
+        'apm-services-telemetry': {


I know I asked for this but I gotta ask: will this cause migration issues?

It doesn't trigger a migration, as it's a different object type, but it would remove saved objects of the old type. This won't be an issue as the object is ephemeral, and only reference when triggered by the usage collector.

sorenlouv

LGTM. You should probably hold-off merging this until #50211 is merged since that's going into 7.5 and we better avoid delaying that with merge conflicts.

- move saved object types and document IDs to constants file - Updated APM Indices endpoints to use the SavedObjectsClient from the legacy request context, and set the apm-indices schema object to be namspace-agnostic.

elasticmachine · 2019-11-13T23:39:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: fca8cef

…legacy request context, and set the apm-indices schema object to be namspace-agnostic - rename apm-telemetry save object mapping -> apm-services-telemetry (elastic#49994) - move saved object types and document IDs to constants file - Updated APM Indices endpoints to use the SavedObjectsClient from the legacy request context, and set the apm-indices schema object to be namspace-agnostic.

…legacy request context, and set the apm-indices schema object to be namspace-agnostic (#50615) - rename apm-telemetry save object mapping -> apm-services-telemetry (#49994) - move saved object types and document IDs to constants file - Updated APM Indices endpoints to use the SavedObjectsClient from the legacy request context, and set the apm-indices schema object to be namspace-agnostic.

dgieselaar · 2020-01-21T11:25:03Z

Functionality was tested as part of #53541.

ogupte added the release_note:skip Skip the PR/issue when compiling release notes label Nov 2, 2019

ogupte requested review from rudolf and legrego November 2, 2019 07:19

ogupte requested a review from a team as a code owner November 2, 2019 07:19

ogupte self-assigned this Nov 2, 2019

ogupte added the v7.6.0 label Nov 2, 2019

sorenlouv reviewed Nov 6, 2019

View reviewed changes

ogupte added v7.5.0 and removed v7.6.0 labels Nov 6, 2019

sorenlouv reviewed Nov 11, 2019

View reviewed changes

x-pack/legacy/plugins/apm/server/lib/helpers/es_client.ts Show resolved Hide resolved

sorenlouv reviewed Nov 11, 2019

View reviewed changes

x-pack/legacy/plugins/apm/server/lib/settings/agent_configuration/create_agent_config_index.ts Show resolved Hide resolved

ogupte force-pushed the apm-49993-saved-objects-client-req-context branch from 6ccb72c to b3a4c08 Compare November 12, 2019 00:30

ogupte added v7.6.0 and removed v7.5.0 labels Nov 12, 2019

ogupte force-pushed the apm-49993-saved-objects-client-req-context branch from b3a4c08 to 561445c Compare November 12, 2019 19:44

sorenlouv reviewed Nov 12, 2019

View reviewed changes

sorenlouv approved these changes Nov 12, 2019

View reviewed changes

ogupte force-pushed the apm-49993-saved-objects-client-req-context branch from 561445c to 81ceef9 Compare November 12, 2019 23:04

ogupte mentioned this pull request Nov 13, 2019

[APM] Don't read APM index names directly from config #49620

Closed

2 tasks

ogupte force-pushed the apm-49993-saved-objects-client-req-context branch from 81ceef9 to fca8cef Compare November 13, 2019 22:19

ogupte merged commit cd67add into elastic:master Nov 14, 2019

ogupte mentioned this pull request Nov 14, 2019

[7.x] Updated APM Indices endpoints to use the SavedObjectsClient from the legacy request context, and set the apm-indices schema object to be namspace-agnostic (#49994) #50615

Merged

ogupte removed their assignment Jan 15, 2020

ogupte added the apm-test-plan-7.6.0 label Jan 15, 2020

dgieselaar self-assigned this Jan 21, 2020

dgieselaar added the apm:test-plan-done Pull request that was successfully tested during the test plan label Jan 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Use SavedObjectsClient from the request context for APM indices endpoints #49994

[APM] Use SavedObjectsClient from the request context for APM indices endpoints #49994

ogupte commented Nov 2, 2019 •

edited by sorenlouv

Loading

elasticmachine commented Nov 2, 2019

sorenlouv commented Nov 3, 2019

sorenlouv Nov 6, 2019 •

edited

Loading

sorenlouv Nov 6, 2019

ogupte Nov 11, 2019

sorenlouv Nov 11, 2019 •

edited

Loading

sorenlouv Nov 11, 2019

sorenlouv Nov 6, 2019

ogupte Nov 11, 2019

sorenlouv Nov 6, 2019

ogupte Nov 11, 2019

sorenlouv Nov 11, 2019

ogupte Nov 12, 2019

sorenlouv Nov 11, 2019 •

edited

Loading

sorenlouv Nov 11, 2019

rudolf Nov 11, 2019

sorenlouv Nov 11, 2019

rudolf Nov 11, 2019

elasticmachine commented Nov 12, 2019

elasticmachine commented Nov 12, 2019

sorenlouv Nov 12, 2019

sorenlouv Nov 12, 2019

ogupte Nov 12, 2019 •

edited

Loading

sorenlouv left a comment

elasticmachine commented Nov 13, 2019

dgieselaar commented Jan 21, 2020

	export const APM_INDICES_SAVED_OBJECT_TYPE = 'apm-indices';
	export const APM_INDICES_SAVED_OBJECT_ID = 'apm-indices';

	const savedObjectsClient = getSavedObjectsClient(server);
	const unscopedSavedObjectsClient = getUnscopedSavedObjectsClient(server);

	const savedObjectsClient = getSavedObjectsClient(server);
	const internalSavedObjectsClient = getInternalSavedObjectsClient(server);

[APM] Use SavedObjectsClient from the request context for APM indices endpoints #49994

[APM] Use SavedObjectsClient from the request context for APM indices endpoints #49994

Conversation

ogupte commented Nov 2, 2019 • edited by sorenlouv Loading

elasticmachine commented Nov 2, 2019

💚 Build Succeeded

sorenlouv commented Nov 3, 2019

sorenlouv Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 12, 2019

💚 Build Succeeded

elasticmachine commented Nov 12, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogupte Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 13, 2019

💚 Build Succeeded

dgieselaar commented Jan 21, 2020

ogupte commented Nov 2, 2019 •

edited by sorenlouv

Loading

sorenlouv Nov 6, 2019 •

edited

Loading

sorenlouv Nov 11, 2019 •

edited

Loading

sorenlouv Nov 11, 2019 •

edited

Loading

ogupte Nov 12, 2019 •

edited

Loading