Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new es client in Uptime usage collector #86718

Merged

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Dec 21, 2020

Summary

Refactors the fetch method in the uptime usage collector (KibanaTelemetryAdapter) to use the new elasticsearch client rather than the deprecated legacy client.
Part of #86358

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@TinaHeiligers TinaHeiligers added Feature:Telemetry v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Dec 21, 2020
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers marked this pull request as ready for review December 22, 2020 18:43
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner December 22, 2020 18:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@TinaHeiligers TinaHeiligers changed the title use new es client in Uptime usage collector Use new es client in Uptime usage collector Dec 23, 2020
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

const savedObjectsClient = getSavedObjectsClient()!;
if (savedObjectsClient) {
await this.countNoOfUniqueMonitorAndLocations(callCluster, savedObjectsClient);
await this.countNoOfUniqueMonitorAndLocations(esClient, savedObjectsClient);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await this.countNoOfUniqueMonitorAndLocations(esClient, savedObjectsClient);
const uptimeEsClient = createUptimeESClient({ esClient, savedObjectsClient });
await this.countNoOfUniqueMonitorAndLocations(uptimeEsClient, savedObjectsClient);

i think doing it like this will simplify things

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 The types for the SavedObjectsClient aren't the same in the collector and in createUptimeEsClient. In the collector, savedObjectsClient is of type ISavedObjectsRepository | undefined, whereas createUptimeEsClient expects savedObjectsClient to be of type SavedObjectsClientContract. We're missing errors in the type that's being passed in.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 we can get around this by changing the savedObjectsClient type expected in createUptimeESClient to be SavedObjectsClientContract | ISavedObjectsRepository. Could you please take a look at the changes and approve or request changes if needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TinaHeiligers yeah i think that can work.

Comment on lines 195 to 197
const { body: result } = isUptimeESClient(callCluster)
? await callCluster.search(params)
: await callCluster.search<ESSearchResponse<unknown, typeof params>>(params);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with your last commit i think, we can simplify this.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 Great thinking, thank you for pointing that out!
I've replaced the conditional here

@@ -132,7 +130,7 @@ export class KibanaTelemetryAdapter {
}

public static async countNoOfUniqueMonitorAndLocations(
callCluster: ILegacyScopedClusterClient['callAsCurrentUser'] | UptimeESClient,
callCluster: ElasticsearchClient | UptimeESClient,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will now only be UptimeESClient

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shahzad31 Thanks for catching that! Done

@TinaHeiligers
Copy link
Contributor Author

@shahzad31 Hopefully the third time reviewing this will be it.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47266 48026 +760

History

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

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !!

@TinaHeiligers TinaHeiligers merged commit ff94674 into elastic:master Jan 5, 2021
@TinaHeiligers TinaHeiligers deleted the uptime-usage-collector-refactor branch January 5, 2021 15:32
TinaHeiligers added a commit that referenced this pull request Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants