[APM] Annotations API #64796

dgieselaar · 2020-04-29T16:30:41Z

I've scoped this down a bit to make sure we get this in for 7.8. Mainly, I'm focused on enabling deployment annotations for APM. I'm deviating from the original issue in a couple of places:

No public search API. There isn't really a public use case right now (that I know of), and users could always fall back to searching the index directly.
annotation.content and annotation.tags have moved to the ECS fields message and tags
The index is not created on startup, as the kibana_system user does not have the appropriate permissions. Instead, the index is created on-the-fly when an annotation is indexed, with the credentials of the user that has triggered the request. This has the added benefit of not creating an index when the user doesn't plan on using annotations.

Rough outline

Adds a server plugin for Observability.
This plugin exposes a factory function that will return an annotations client when annotations are enabled (and by default they are).
The annotations client takes care of searching, creating and deleting annotations.
The Observability server plugin exposes three (untagged) API endpoints, for creating an annotation, and searching or deleting by id. These endpoints can be accessed from outside of Kibana via API keys or basic auth.
When an annotation is created, the index is created on the fly with the requesting user's privileges.
Searching for annotations will currently only return the first page, and size is configurable. No pagination is possible.
The APM server plugin adds an optional dependency on the Observability server plugin, and adds another endpoint POST /api/apm/services/{serviceName}/annotation that will use the annotations client to create an annotation. It will prefill/override certain fields, like annotation.type, tags and service.name.
When the APM App requests annotations, two requests will run in parallel: one that will get the derived deployment annotations, as we have today, and another to get the persisted annotations. If there are any persisted annotations, it will only show those.

dgieselaar · 2020-04-29T19:27:12Z

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

@@ -205,7 +205,9 @@ export function getESClient(
    index: <Body>(params: APMIndexDocumentParams<Body>) => {
      return callEs('index', params);
    },
-    delete: (params: IndicesDeleteParams): Promise<DeleteDocumentResponse> => {
+    delete: (
+      params: Omit<DeleteDocumentParams, 'type'>


this deletes a document, not an index, so I've added the correct type.

dgieselaar · 2020-04-29T19:28:05Z

x-pack/plugins/apm/server/lib/services/annotations/get_derived_service_annotations.ts

+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */


This is what we previously had (derived annotations), moved to separate file.

dgieselaar · 2020-04-29T20:14:55Z

x-pack/plugins/apm/server/lib/services/annotations/index.ts

+  const [derivedAnnotations, storedAnnotations] = await Promise.all([
+    getDerivedServiceAnnotations({
+      setup,
+      serviceName,
+      environment
+    }),
+    getStoredAnnotations()
+  ]);


I might change this to something that will start both requests, but return as soon as the stored annotations are resolved. The aggregation to get the derived annotations can be quite costly.

dgieselaar · 2020-04-29T20:17:29Z

x-pack/plugins/apm/server/routes/services.ts

+  path: '/api/apm/services/{serviceName}/annotation',
+  method: 'POST',
+  options: {
+    tags: ['access:apm', 'access:apm_write']


Users can easily work around this by writing to the index directly, but I wanted to keep things consistent with the agent config APIs. Thoughts?

Thoughts around the security model? I think this is good. Now at least it's possible to lock down the index and only give api access.

... oh, you can pass in tags.. yeah not sure I understand that part.

you mean tags on an annotation? or tags on the route?

Disregard the above comments. tags for annotations make sense 👍

dgieselaar · 2020-04-29T20:21:08Z

x-pack/plugins/observability/server/lib/annotations/bootstrap_annotations.ts

+export async function bootstrapAnnotations({ index, core, context }: Params) {
+  const logger = context.logger.get('annotations');
+
+  function wrapRouteHandler<TType extends t.Type<any>>(


This needs some work I think. Open to suggestions to make it better.

Perhaps extract everything related to the API into a create_anotations_api.ts so it's clear that bootstrap_annotations.ts creates the API and a returns a client as well.

moved it into register_annotation_apis (named it as such to distinguish it from create_annotations_client)

dgieselaar · 2020-04-29T20:21:51Z

x-pack/plugins/observability/server/utils/create_or_update_index.ts

@@ -0,0 +1,108 @@
+/*


this file was moved from apm to observability.

Closes elastic#58320.

dgieselaar · 2020-04-30T09:07:20Z

x-pack/plugins/observability/server/lib/annotations/create_annotations_client.ts

+    });
+
+  return {
+    search: async (searchParams: SearchParams): Promise<SearchResponse<Annotation>> => {


Right now this is just a thin wrapper around _search. I've added it because we are planning on querying multiple sources in the near future (for instance, the event log). I was talking to @sqren about this and we wonder if we should even have it, or expect consumers of annotations to query the index directly. Having validation when indexing documents is definitely valuable but do we need a (limiting) abstraction around searching as well?

I think it's better to remove it until there is a need.

removed, APM now talks to ES directly to get the stored annotations

cauemarcondes · 2020-05-04T14:28:44Z

x-pack/plugins/apm/server/routes/services.ts

@@ -91,10 +95,77 @@ export const serviceAnnotationsRoute = createRoute(() => ({
    const { serviceName } = context.params.path;
    const { environment } = context.params.query;

+    let annotationsClient: ScopedAnnotationsClient | undefined;


Can't you do it like this?

Suggested change

let annotationsClient: ScopedAnnotationsClient | undefined;

const annotationsClient = await context.plugins.observability?.getScopedAnnotationsClient(

request

);

Changed, thanks!

andrewvc · 2020-05-04T14:37:39Z

I'm very excited about annotations! I haven't had a chance to do a deep dive into the code, but I do have some security related questions:

Is this space aware at all? Have we discussed the security considerations for annotations made in one space being viewable in another? Were saved objects ruled out here?

Is it correct that we will need to update our security docs to include index creation permissions for Kibana observability users?

dgieselaar · 2020-05-04T15:12:11Z

@andrewvc It's not space-aware at this point. Saved objects were considered, but if I remember correctly, ECS compatibility and ILM possibilities were some of the reasons why we decided to go with a custom index. Another one could be ease of querying.

Indeed documentation should be updated, I'll make sure to sync with @bmorelli25 about what needs to be updated where.

sorenlouv · 2020-05-04T15:29:31Z

x-pack/plugins/apm/public/context/ChartsSyncContext.tsx

@@ -28,7 +28,7 @@ const ChartsSyncContextProvider: React.FC = ({ children }) => {
    callApmApi => {
      if (start && end && serviceName) {
        return callApmApi({
-          pathname: '/api/apm/services/{serviceName}/annotations',
+          pathname: '/api/apm/services/{serviceName}/annotation/search',


I know this was not introduced in this PR but...
it seems wrong to fetch annotations data here. The responsibility of ChartsSyncContext was simply to keep the hover state in sync between multiple charts.

What about pulling the annotations fetching out to a separate hook and call it where needed? I suspect it's only two places:

kibana/x-pack/plugins/apm/public/components/shared/charts/TransactionCharts/TransactionLineChart/index.tsx

Line 45 in 7002fa8

const syncedChartsProps = useChartsSync();

kibana/x-pack/plugins/apm/public/components/shared/charts/MetricsChart/index.tsx

Line 40 in 7002fa8

const syncedChartProps = useChartsSync();

I've thought about this for a bit but I'm not sold. To me, ChartsSyncContext is just a place where we store state common to all charts. I'm not sure if splitting this up would improve things, sounds easy to forget. Do you have any specific concerns, other than responsibility?

sorenlouv · 2020-05-04T15:36:05Z

x-pack/plugins/apm/server/lib/services/annotations/get_derived_service_annotations.ts

+        index: indices['apm_oss.transactionIndices'],
+        body: {
+          size: 0,
+          track_total_hits: false,


This should be the default so should not matter

The default is 10000 rather than false. See the responses for not setting it and setting it to false:

{ "took" : 0, "timed_out" : false, "_shards" : { "total" : 1, "successful" : 1, "skipped" : 0, "failed" : 0 }, "hits" : { "total" : { "value" : 10000, "relation" : "gte" }, "max_score" : null, "hits" : [ ] } }

vs

{ "took" : 0, "timed_out" : false, "_shards" : { "total" : 1, "successful" : 1, "skipped" : 0, "failed" : 0 }, "hits" : { "max_score" : null, "hits" : [ ] } }

I'll remove it though, it won't make any significant difference in terms of performance.

ah, didn't think about that. Good point 👍

sorenlouv · 2020-05-04T15:40:53Z

x-pack/plugins/apm/server/lib/services/annotations/get_derived_service_annotations.ts

+                filter: filter
+                  .filter(esFilter => !Object.keys(esFilter).includes('range'))
+                  .concat({
+                    term: {
+                      [SERVICE_VERSION]: version
+                    }
+                  })


I think this would be a lot simpler to read:

[ { term: { [PROCESSOR_EVENT]: 'transaction' } }, { term: { [SERVICE_NAME]: serviceName } }, { term: { [SERVICE_VERSION]: version } } ]

(I ❤️ boring, repetitive code )

I've now also added slightly more complicated handling of the environment filter. I'll simplify this to two concats, LMK what you think.

sorenlouv · 2020-05-04T16:10:51Z

x-pack/plugins/apm/server/lib/services/annotations/get_derived_service_annotations.ts

+      })
+    ).aggregations?.versions.buckets.map(bucket => bucket.key) ?? [];
+
+  if (versions.length > 1) {


Do you mind flipping this around. Putting the short branch first makes it possible to understand the full picture immediately and dedent the second branch

if (versions.length === 0) { return { annotations: [] }; }

I secretly call the other approach "cliffhanger-code" because I feel like I have to cheat when I jump to the bottom to figure out the ending :p

sorenlouv · 2020-05-04T16:11:49Z

x-pack/plugins/apm/server/lib/services/annotations/get_derived_service_annotations.ts

+                }
+              }
+            },
+            track_total_hits: false


Not needed (afaik)

sorenlouv · 2020-05-04T16:14:07Z

x-pack/plugins/apm/server/lib/services/annotations/get_derived_service_annotations.ts

+  SERVICE_VERSION
+} from '../../../../common/elasticsearch_fieldnames';
+
+export async function getDerivedServiceAnnotations({


Perhaps add a comment here that explains what a derived annotation is (compared to a normal annotation)

sorenlouv · 2020-05-04T16:17:33Z

x-pack/plugins/apm/server/lib/services/annotations/get_derived_service_annotations.ts

+        return {
+          type: AnnotationType.VERSION,
+          id: version,
+          time: firstSeen,


don't we normally call this timestamp? or timestamp.us to indicate the precision?

changed to @timestamp

sorenlouv · 2020-05-04T18:11:23Z

x-pack/plugins/observability/server/lib/annotations/create_annotations_client.ts

+            },
+          },
+        },
+      },


It's quite hard to read - perhaps extract mappings

sorenlouv · 2020-05-04T18:12:18Z

x-pack/plugins/observability/server/lib/annotations/create_annotations_client.ts

+            successful: 0,
+            total: 0,
+          },
+          timed_out: false,


Faking an ES response like this feels wrong.

removed (along with the search method)

sorenlouv · 2020-05-04T18:13:26Z

x-pack/plugins/observability/server/lib/annotations/create_annotations_client.ts

+    create: async (
+      createParams: CreateParams
+    ): Promise<{ _id: string; _index: string; _source: Annotation }> => {
+      await initIndex();


Could it have unintended consequences to update the mappings every time an annotation is created ?

I'll add a indices.exists call before.

sorenlouv · 2020-05-04T18:52:01Z

x-pack/plugins/observability/server/plugin.ts

+        index: config.annotations.index,
+        context: this.initContext,
+      }).catch(err => {
+        this.initContext.logger.get('annotations').warn(err);


I think splitting it into two lines will make it look more familiar to most people.

Suggested change

this.initContext.logger.get('annotations').warn(err);

const logger = this.initContext.logger.get('annotations');

logger.warn(err);

sorenlouv · 2020-05-04T18:54:47Z

x-pack/plugins/observability/server/plugin.ts

+        return annotationsApiPromise
+          ? (await annotationsApiPromise).getScopedAnnotationsClient(...args)
+          : undefined;


nit:

Suggested change

return annotationsApiPromise

? (await annotationsApiPromise).getScopedAnnotationsClient(...args)

: undefined;

const api = await annotationsApiPromise;

return api?.getScopedAnnotationsClient(...args);

sorenlouv · 2020-05-04T22:27:53Z

x-pack/test/api_integration/apis/apm/annotations.ts

+      });
+    });
+  });
+}


Thorough test 👍 I bet it made you think twice about the API design :)

Kerry350

LGTM 👍

Left one minor comment, needn’t hold up merging though.

Kerry350 · 2020-05-05T10:30:37Z

x-pack/plugins/observability/server/lib/annotations/bootstrap_annotations.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+import { CoreSetup, PluginInitializerContext, KibanaRequest } from 'kibana/server';
+import { PromiseReturnType } from '../../../../apm/typings/common';


Super minor nit: I think it’d be nice to keep imports from other plugins to a minimum, or from a location that denotes sharing. Could this type be added to the Observability plugin directly? Or exported from the apm plugin in one of the locations that denotes it’s part of the shared / public contract?

Moving PromiseReturnType to the observability plugin sounds like a good idea 👍

skh

👍 from Ingest Management, looking forward to learning more about annotations!

kibanamachine · 2020-05-05T14:25:49Z

💚 Build Succeeded

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

History

💚 Build #45438 succeeded e941a37
💔 Build #45433 failed 983222b
💚 Build #44778 succeeded 292bf95
💚 Build #44358 succeeded 15301b2
💔 Build #44339 failed 232c1922b547928e4e5af17f6561a52cd02985f8

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

andrewvc · 2020-05-05T15:15:03Z

@dgieselaar What is the access control story we want to tell? It seems odd that we would let people segment their data completely with RBAC and spaces but annotations would cross those boundaries.

The additional index maxes sense, but something here feels weird from an administrative standpoint. If I were an operator trying to reason about my indices this one feels like the odd one out. If this is the best option that makes sense, but having just reworked our security docs this adds a whole new dimension of complexity that worries me.

dgieselaar · 2020-05-05T15:22:07Z

@andrewvc I think we can make this space-aware in the future by storing the index name in a saved object, both for reading and writing. The APM app is not space aware so I figure we'd start out simple. Do you have any concerns around merging this in for 7.7 (it will be marked as Beta), and figuring out access control later?

sorenlouv · 2020-05-05T16:34:33Z

@dgieselaar How hard would it be to make this space-aware for 7.8? I can live without it for now but I just want to make sure this doesn't force us do migration or other things to preserve backwards compatibility when we do add spaces and RBAC

dgieselaar · 2020-05-05T16:59:06Z

@sqren I don't think it would be particularly hard in terms of implementation. index, which is now a sync getter, would need to become an async operation where it reads from the (user's?) saved objects repository. But, it would probably need some UI, and functional tests, and I don't feel comfortable making a change like that this close to FF. I would prefer us to document any migration scenarios if we should need them.

dgieselaar · 2020-05-05T17:49:16Z

@andrewvc I've created an issue to track the access control issue. I'll merge this in now given the time constraints (apologies 😃)

andrewvc · 2020-05-05T19:07:56Z

I don't think any of the concerns I've raised need to block this PR, but we revisit the decisions here at some point to inform a more ergonomic way of defining privileges in the future.

dgieselaar added release_note:enhancement v7.8.0 labels Apr 29, 2020

dgieselaar commented Apr 29, 2020

View reviewed changes

dgieselaar force-pushed the annotations-api branch from 5afd149 to 232c192 Compare April 30, 2020 07:18

[APM] Annotations API

15301b2

Closes elastic#58320.

dgieselaar force-pushed the annotations-api branch from 232c192 to 15301b2 Compare April 30, 2020 08:46

dgieselaar commented Apr 30, 2020

View reviewed changes

dgieselaar marked this pull request as ready for review April 30, 2020 10:00

dgieselaar requested a review from a team as a code owner April 30, 2020 10:00

dgieselaar requested a review from a team April 30, 2020 10:00

dgieselaar requested a review from a team as a code owner April 30, 2020 10:00

dgieselaar added 2 commits April 30, 2020 21:57

Merge branch 'master' of github.com:elastic/kibana into annotations-api

3af9d03

Merge branch 'master' of github.com:elastic/kibana into annotations-api

292bf95

cauemarcondes reviewed May 4, 2020

View reviewed changes

sorenlouv reviewed May 4, 2020

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into annotations-api

ba3a85b

dgieselaar added 4 commits May 5, 2020 10:38

Simplify query structure for getDerivedServiceAnnotations

a75b37a

Simplify query structure for getDerivedServiceAnnotations

1dd7a73

Remaining feedback

983222b

Update observability API tests

e941a37

Kerry350 self-requested a review May 5, 2020 10:16

dgieselaar requested review from sorenlouv and cauemarcondes May 5, 2020 10:26

Kerry350 approved these changes May 5, 2020

View reviewed changes

skh approved these changes May 5, 2020

View reviewed changes

sorenlouv approved these changes May 5, 2020

View reviewed changes

Move PromiseReturnType to observability typings

dd6348c

dgieselaar mentioned this pull request May 5, 2020

[Observabilty] Access control for annotations #65314

Closed

dgieselaar merged commit 399eed7 into elastic:master May 5, 2020

dgieselaar deleted the annotations-api branch May 5, 2020 17:49

dgieselaar mentioned this pull request May 5, 2020

[7.x] [APM] Annotations API (#64796) #65317

Merged

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request May 5, 2020

[APM] Annotations API (elastic#64796)

091f75a

dgieselaar added a commit that referenced this pull request May 5, 2020

[7.x] [APM] Annotations API (#64796) (#65317)

1f2cc2f

bmorelli25 mentioned this pull request May 11, 2020

[apm] Annotation API documentation #65963

Merged

sorenlouv added the apm:test-plan-7.8.0 label May 13, 2020

ogupte self-assigned this Jun 5, 2020

ogupte added the apm:test-plan-done Pull request that was successfully tested during the test plan label Jun 5, 2020

dgieselaar mentioned this pull request Jun 25, 2020

Add read privileges for observability-annotations for apm_user elastic/elasticsearch#58530

Merged

andrewvc mentioned this pull request Aug 4, 2020

Add Chart annotations elastic/uptime#244

Open

	this.initContext.logger.get('annotations').warn(err);
	const logger = this.initContext.logger.get('annotations');
	logger.warn(err);

[APM] Annotations API #64796

[APM] Annotations API #64796

Conversation

dgieselaar commented Apr 29, 2020 • edited Loading

Rough outline

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv May 4, 2020 • 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

andrewvc commented May 4, 2020

dgieselaar commented May 4, 2020

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 May 4, 2020 • 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

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 May 4, 2020 • 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

Kerry350 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skh left a comment

Choose a reason for hiding this comment

kibanamachine commented May 5, 2020

💚 Build Succeeded

History

andrewvc commented May 5, 2020

dgieselaar commented May 5, 2020

sorenlouv commented May 5, 2020 • edited Loading

dgieselaar commented May 5, 2020 • edited Loading

dgieselaar commented May 5, 2020

andrewvc commented May 5, 2020

dgieselaar commented Apr 29, 2020 •

edited

Loading

sorenlouv May 4, 2020 •

edited

Loading

sorenlouv May 4, 2020 •

edited

Loading

sorenlouv May 4, 2020 •

edited

Loading

sorenlouv commented May 5, 2020 •

edited

Loading

dgieselaar commented May 5, 2020 •

edited

Loading