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

refresh cache on unencrypted telemetry request #124253

Merged
merged 8 commits into from
Feb 2, 2022
Merged

refresh cache on unencrypted telemetry request #124253

merged 8 commits into from
Feb 2, 2022

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented Feb 1, 2022

Closes #123021

There is an unrelated i18n fix to have CI pass when we canvas/shareable_runtime directory is built with webpack

@Bamieh Bamieh added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 v8.2.0 labels Feb 1, 2022
@Bamieh Bamieh requested review from a team as code owners February 1, 2022 14:57
afharo
afharo reviewed Feb 1, 2022
View reviewed changes
src/plugins/telemetry/server/routes/telemetry_usage_stats.ts
@@ -35,7 +35,7 @@ export function registerTelemetryUsageStatsRoutes(
const statsConfig: StatsGetterConfig = {
request: req,
unencrypted,
refreshCache,
refreshCache: unencrypted || refreshCache,
Copy link
Member

Choose a reason for hiding this comment

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

Q: Should this be handled down inside the telemetryCollectionManager.getStats? It may cause different behaviours if called from somewhere else. What do you think?

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'd like to keep the logic at the route level since we might want to set up things differently in different places

Copy link
Member

Choose a reason for hiding this comment

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

I see where you're coming from. However, the cache is implemented in telemetryCollectionManager.getStats, I don't see why others may want to use it in a different way. Especially bearing in mind that, getStats is the one deciding how to scope the requests (so any other place using unencrypted: true, refreshCache: false might expose again values retrieved with different permissions to the caller.

IMO, the scoping and the refresh algos should be together.

@Bamieh Bamieh requested a review from afharo February 1, 2022 19:05
afharo
afharo approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@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.

LGTM! Thank you @Bamieh 🔥

src/plugins/telemetry_collection_manager/server/mocks.ts
@@ -0,0 +1,39 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

🧡

@Bamieh Bamieh enabled auto-merge (squash) February 2, 2022 12:54
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #21 / transform creation_index_pattern batch transform with terms group and terms agg navigates through the wizard and sets all needed fields

Metrics [docs]

✅ unchanged

History

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

@Bamieh Bamieh merged commit fcac5d6 into elastic:main Feb 2, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.2 The branch "8.2" is invalid or doesn't exist

How to fix

Re-run the backport manually:

node scripts/backport --pr 124253

Questions ?

Please refer to the Backport tool documentation

@Bamieh Bamieh deleted the telemetry_unencrypted branch February 2, 2022 14:35
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 4, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124253 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 124253 or prevent reminders by adding the backport:skip label.

@Bamieh Bamieh added the backport:skip This commit does not require backporting label Feb 8, 2022
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afharo afharo afharo approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.1.0 v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Telemetry] The caching mechanism also caches failed payloads
4 participants
@Bamieh @kibana-ci @kibanamachine @afharo