SavedObjectsRepository.incrementCounter supports array of fields #84326

rudolf · 2020-11-25T12:46:42Z

Plugin API changes

The SavedObjectsRepository.incrementCounter method no longer accepts a string field name, an array of field names to increment must be provided.

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-11-26T07:17:49Z

Pinging @elastic/kibana-core (Team:Core)

flash1293

KibanaApp changes LGTM, didn't test

afharo

Telemetry changes LGTM!

src/core/server/saved_objects/service/lib/repository.ts

afharo · 2020-11-26T09:05:09Z

src/core/server/saved_objects/service/lib/repository.ts

+   * When using incrementCounter for collecting usage data, you need to ensure
+   * that usage collection happens on a best-effort basis and doesn't
+   * negatively affect your plugin or users (see the example):
+   *  - Swallow any exceptions thrown from the incrementCounter method and log
+   *    a message in development.
+   *  - Don't block your application on the incrementCounter method (e.g.
+   *    don't use `await`)


I love this remark 🧡

Isn't that a bit misplaced though? I would expect that kind of comment/guidelines to be in the usage collection plugin's README instead of being documented in a core API?

yeah I agree, this doesn't really belong in the SavedObjectsRepository's documentation. But I everywhere where this method was called teams awaited this promise and I'm afraid they won't read the docs in the plugin's readme.

We could maybe move this to the usage data plugin's docs and add a link to the docs. Because the link will be from tsdocs -> markdown it might be best to link to the github url for that file. WDYT?

Yea, I would go with something like

When using incrementCounter for collecting usage data, you need to ensure that usage collection happens on a best-effort basis and doesn't negatively affect your plugin or users. Please refer to the [usage collection documentation](link) for more details.

jloleysens

Upgrade Assistant changes look good to me!

streamich

AppArch changes LGTM.

scottybollinger

LGTM from Enterprise Search

src/core/server/saved_objects/service/lib/repository.ts

pgayvallet · 2020-11-29T18:51:38Z

src/core/server/saved_objects/service/lib/repository.ts

+   * When using incrementCounter for collecting usage data, you need to ensure
+   * that usage collection happens on a best-effort basis and doesn't
+   * negatively affect your plugin or users (see the example):
+   *  - Swallow any exceptions thrown from the incrementCounter method and log
+   *    a message in development.
+   *  - Don't block your application on the incrementCounter method (e.g.
+   *    don't use `await`)


Isn't that a bit misplaced though? I would expect that kind of comment/guidelines to be in the usage collection plugin's README instead of being documented in a core API?

…ection plugins readme

kibanamachine · 2020-11-30T22:45:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 308a098

Metrics [docs]

✅ unchanged

History

💚 Build #90867 succeeded d268882
💔 Build #90758 failed c196193
💚 Build #90437 succeeded 7643c19
💛 Build #90250 was flaky 5e2c7f3
💔 Build #90239 failed 8ba65bc

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

jportner · 2020-12-01T01:49:54Z

@rudolf We spoke offline about adding a flag to initialize specified fields to 0.

In addition to that: while implementing #80945, I've found another improvement we could make to the incrementCounter method.

When a "legacy URL alias" is resolved, we want to increment a counter and update a timestamp. I thought I would mention it here in case you think it makes sense to add support for updating a timestamp field in this PR.

pgayvallet

initialize option changes LGTM

rudolf · 2020-12-01T10:28:16Z

When a "legacy URL alias" is resolved, we want to increment a counter and update a timestamp. I thought I would mention it here in case you think it makes sense to add support for updating a timestamp field in this PR.

We could certainly add a parameter like timestampFieldNames. Though it feels like we would need to rename the method since we're no longer just incrementing a counter. Something like scriptedUpdate(type: string, id: string: actions: [{type: 'increment_counter'|'update_timestamp', field: string}]) might be a nice abstraction so that we can later add additional useful action types which require scripting.

For "legacy URL alias" do we require a generic public method on the repository (like incrementCounter/scriptedUpdate), or could we put this scripting inside the resolve() method?

) (#84613) * SavedObjectsRepository.incrementCounter supports array of fields * Fix TS errors * Fix failing test * Ensure all the remarks make it into our documentation * SavedObjectsRepository.incrementCounter initialize option * Move usage collection-specific docs out of repository into usage collection plugins readme * Update api docs * Polish generated docs

jportner · 2020-12-01T14:01:00Z

For "legacy URL alias" do we require a generic public method on the repository (like incrementCounter/scriptedUpdate), or could we put this scripting inside the resolve() method?

Good point, we don't really need a generic public method for this!

bhavyarm · 2021-01-18T21:50:17Z

@rudolf how do I test this please? Thanks!

rudolf · 2021-01-19T11:01:20Z

@bhavyarm this changes the shape and functionality of an API that I think is exclusively used for telemetry so there's no user-facing changes that can be tested.

SavedObjectsRepository.incrementCounter supports array of fields

7cb73eb

rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Feature:Saved Objects v7.11.0 labels Nov 25, 2020

Fix TS errors

8ba65bc

Fix failing test

5e2c7f3

rudolf marked this pull request as ready for review November 26, 2020 07:17

rudolf requested a review from a team November 26, 2020 07:17

rudolf requested a review from a team as a code owner November 26, 2020 07:17

rudolf requested a review from a team November 26, 2020 07:17

rudolf requested review from a team as code owners November 26, 2020 07:17

rudolf requested a review from a team November 26, 2020 07:17

flash1293 approved these changes Nov 26, 2020

View reviewed changes

afharo approved these changes Nov 26, 2020

View reviewed changes

jloleysens approved these changes Nov 26, 2020

View reviewed changes

rudolf mentioned this pull request Nov 26, 2020

[SAVED_OBJECTS] [AGGS] Step I to add aggregations in the find of saved objects #64002

Closed

3 tasks

Ensure all the remarks make it into our documentation

7643c19

streamich approved these changes Nov 26, 2020

View reviewed changes

ryankeairns approved these changes Nov 27, 2020

View reviewed changes

scottybollinger approved these changes Nov 27, 2020

View reviewed changes

pgayvallet reviewed Nov 29, 2020

View reviewed changes

rudolf mentioned this pull request Nov 30, 2020

Disable checking for conflicts when copying saved objects #83575

Merged

pgayvallet approved these changes Nov 30, 2020

View reviewed changes

rudolf added 3 commits November 30, 2020 15:47

SavedObjectsRepository.incrementCounter initialize option

8f0a3ae

Merge branch 'master' into incrementcounter

668fe4c

Move usage collection-specific docs out of repository into usage coll…

c196193

…ection plugins readme

rudolf added 2 commits November 30, 2020 20:56

Update api docs

d268882

Polish generated docs

308a098

pgayvallet approved these changes Dec 1, 2020

View reviewed changes

rudolf merged commit 4507865 into elastic:master Dec 1, 2020

rudolf deleted the incrementcounter branch December 1, 2020 10:28

rudolf mentioned this pull request Dec 1, 2020

[7.x] SavedObjectsRepository.incrementCounter supports array of fields (#84326) #84613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SavedObjectsRepository.incrementCounter supports array of fields #84326

SavedObjectsRepository.incrementCounter supports array of fields #84326

rudolf commented Nov 25, 2020 •

edited

Loading

elasticmachine commented Nov 26, 2020

flash1293 left a comment

afharo left a comment

afharo Nov 26, 2020

pgayvallet Nov 29, 2020

rudolf Nov 30, 2020

pgayvallet Nov 30, 2020

rudolf Nov 30, 2020

jloleysens left a comment

streamich left a comment

scottybollinger left a comment

pgayvallet Nov 29, 2020

kibanamachine commented Nov 30, 2020

jportner commented Dec 1, 2020

pgayvallet left a comment

rudolf commented Dec 1, 2020

jportner commented Dec 1, 2020

bhavyarm commented Jan 18, 2021

rudolf commented Jan 19, 2021

SavedObjectsRepository.incrementCounter supports array of fields #84326

SavedObjectsRepository.incrementCounter supports array of fields #84326

Conversation

rudolf commented Nov 25, 2020 • edited Loading

Plugin API changes

For maintainers

elasticmachine commented Nov 26, 2020

flash1293 left a comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

afharo Nov 26, 2020

Choose a reason for hiding this comment

pgayvallet Nov 29, 2020

Choose a reason for hiding this comment

rudolf Nov 30, 2020

Choose a reason for hiding this comment

pgayvallet Nov 30, 2020

Choose a reason for hiding this comment

rudolf Nov 30, 2020

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

scottybollinger left a comment

Choose a reason for hiding this comment

pgayvallet Nov 29, 2020

Choose a reason for hiding this comment

kibanamachine commented Nov 30, 2020

💚 Build Succeeded

Metrics [docs]

History

jportner commented Dec 1, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

rudolf commented Dec 1, 2020

jportner commented Dec 1, 2020

bhavyarm commented Jan 18, 2021

rudolf commented Jan 19, 2021

rudolf commented Nov 25, 2020 •

edited

Loading