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.

Sign up for GitHub

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

Jump to bottom

[Telemetry] Remove all usages of any #98338

Merged
merged 14 commits into from
Apr 27, 2021
Merged

[Telemetry] Remove all usages of any #98338

merged 14 commits into from
Apr 27, 2021

Conversation

afharo
Copy link
Member

@afharo afharo commented Apr 26, 2021 •
edited
Loading

Summary

Part of #93183: it removes all usages of any in favour of more explicit type definitions.

As a side effect, the Advanced Settings Field component had its types updated to match the running logic: settings.description can also be a React component.

Checklist

For maintainers

@afharo afharo added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels Apr 26, 2021
@afharo afharo mentioned this pull request Apr 26, 2021
Closed
8 tasks
@afharo afharo self-assigned this Apr 26, 2021
@afharo afharo force-pushed the remove-any/kbn-analytics branch from 7685387 to efa6e35 Compare April 26, 2021 17:32
@afharo afharo changed the title @kbn/analytics: Remove all usages of any Remove all usages of any in @kbn/analytics and src/plugins/kibana_usage_collection Apr 26, 2021
@afharo afharo changed the title Remove all usages of any in @kbn/analytics and src/plugins/kibana_usage_collection [Telemetry] Remove all usages of any Apr 27, 2021
@afharo afharo force-pushed the remove-any/kbn-analytics branch from 23805d8 to 208437a Compare April 27, 2021 11:20
afharo
afharo commented Apr 27, 2021
View reviewed changes
Copy link
Member Author

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Self-review

.eslintrc.js
{
files: [
'packages/kbn-analytics/**',
// 'packages/kbn-telemetry-tools/**',
Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving this one for a PR on its own, since it requires a lot of changes.

src/plugins/advanced_settings/public/management_app/types.ts
import { UiCounterMetricType } from '@kbn/analytics';
import { UiSettingsType, StringValidation, ImageValidation } from '../../../../core/public';

export interface FieldSetting {
displayName: string;
name: string;
value: unknown;
description?: string;
description?: string | ReactElement;
Copy link
Member Author

Choose a reason for hiding this comment

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

This change may become handy for #56498. The logic already handles both types, it was just the types that was outdated:

https://github.com/elastic/kibana/blob/master/src/plugins/advanced_settings/public/management_app/components/field/field.tsx#L537-L549

src/plugins/telemetry_collection_manager/server/plugin.ts
Comment on lines +220 to +221
private async getStats(config: UnencryptedStatsGetterConfig): Promise<UsageStatsPayload[]>;
private async getStats(config: EncryptedStatsGetterConfig): Promise<string[]>;
Copy link
Member Author

Choose a reason for hiding this comment

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

this overload helps in getting the right expected type depending on the usage.

src/plugins/usage_collection/server/routes/stats/stats.ts
@@ -55,9 +60,9 @@ export function registerStatsRoute({
}) {
const getUsage = async (
esClient: ElasticsearchClient,
savedObjectsClient: SavedObjectsClientContract | ISavedObjectsRepository,
savedObjectsClient: SavedObjectsClientContract,
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why TS didn't complain earlier. We migrated to SavedObjectsClientContract-only way back in #93289 🤷

@afharo afharo mentioned this pull request Apr 27, 2021
Closed
@afharo afharo marked this pull request as ready for review April 27, 2021 19:02
@afharo afharo requested a review from a team April 27, 2021 19:02
@afharo afharo requested review from a team as code owners April 27, 2021 19:02
Bamieh
Bamieh approved these changes Apr 27, 2021
View reviewed changes
Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM. I've walked through the changes over github but haven't pulled the code locally. Thanks this is a huge effort

@afharo afharo added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 27, 2021
@afharo afharo enabled auto-merge (squash) April 27, 2021 19:20
wylieconlon
wylieconlon approved these changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Did not review the change as a whole, but the small addition to Kibana App code LGTM. Please update the description of this PR to indicate that you've followed the template.

@afharo afharo merged commit 6ed93c3 into elastic:master Apr 27, 2021
@afharo afharo deleted the remove-any/kbn-analytics branch April 27, 2021 19:45
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
❌ 7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 98338

@afharo afharo mentioned this pull request Apr 28, 2021
Merged
afharo added a commit to afharo/kibana that referenced this pull request Apr 28, 2021
@afharo
[Telemetry] Remove all usages of any (elastic#98338)
094d201 
# Conflicts:
#	src/plugins/telemetry/server/collectors/usage/ensure_deep_object.ts
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
telemetry 37 78 +41

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
telemetry 0 1 +1

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
telemetry 3 0 -3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
telemetryManagementSection 22.9KB 23.0KB +90.0B
Unknown metric groups

API count

id before after diff
telemetry 41 83 +42

History

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

cc @afharo

afharo added a commit that referenced this pull request Apr 28, 2021
@afharo
[Telemetry] Remove all usages of any (#98338) (#98583)
17dda21 
# Conflicts:
#	src/plugins/telemetry/server/collectors/usage/ensure_deep_object.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@streamich streamich streamich approved these changes

@Bamieh Bamieh Bamieh approved these changes

@wylieconlon wylieconlon wylieconlon approved these changes

Assignees

@afharo afharo

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@afharo @kibanamachine @wylieconlon @Bamieh @streamich