-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[POC] Event-based Telemetry #95960
[POC] Event-based Telemetry #95960
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review
* @private | ||
*/ | ||
private ensureQueueSize() { | ||
while (this.queue.length > 1 && this.size > this.maxByteSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the only place that we check this.maxByteSize
, we might be able to dynamically update the value once we implement the custom disabling of queues. i.e.: If a plugin registers 3 channels but we disable 2 of them, the remainder channel can get all the plugin's allowance for itself instead of the initial 33%.
this.queue.push(buffer); | ||
this.ensureQueueSize(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: a public clear
method would be useful for:
- Clearing queues when opted-out
- Clearing queues when a channel is disabled
* Side Public License, v 1. | ||
*/ | ||
|
||
import * as t from 'io-ts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using io-ts
as the validation tool because it's orders of magnitude more performant than joi
/@kbn/config-schema
*/ | ||
export type TelemetrySchemaValue = | ||
| { | ||
type: AllowedSchemaTypes | 'pass_through'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially intentionally allowing pass_through
for specific use cases where we want a bit of a weaker validation.
| { | ||
type: AllowedSchemaTypes | 'pass_through'; | ||
_meta: { | ||
description: string; // Intentionally enforcing the descriptions here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description will be mandatory on the leaves
src/plugins/telemetry/server/event_based_telemetry/event_based_telemetry_service.ts
Outdated
Show resolved
Hide resolved
src/plugins/telemetry/server/event_based_telemetry/leaky_bucket.ts
Outdated
Show resolved
Hide resolved
|
||
private isFullQueue(): boolean { | ||
const queueSize = this.queue.reduce((acc, buffer) => acc + buffer.length, 0); | ||
return queueSize >= this.config.threshold.getValueInBytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Possible, performance improvement: precalculate this.config.threshold.getValueInBytes()
into a private variable to avoid calling it on every loop.
private hasMaxWaitExpired(): boolean { | ||
const diff = moment().diff(this.lastSend, 'milliseconds'); | ||
return diff >= this.maxWaitInMs; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Maybe Date.now()
is more performant? We don't call it that often though to be a problem.
src/plugins/telemetry/server/event_based_telemetry/leaky_bucket.ts
Outdated
Show resolved
Hide resolved
43e34fd
to
b90fdec
Compare
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / general / Jest Integration Tests.src/core/server/logging/integration_tests.RollingFileAppender `size-limit` policy with `numeric` strategy rolls the log file in the correct orderStandard Out
Stack Trace
Kibana Pipeline / general / Jest Integration Tests.src/core/server/logging/integration_tests.RollingFileAppender `size-limit` policy with `numeric` strategy only keep the correct number of filesStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
Closing as this POC is no longer relevant. |
Summary
This PR implements a POC for generally available Event-based telemetry APIs.
The reasoning for adding this new set of APIs will be further explained in a subsequent RFC. The TL;DR reason: there are some use cases where we need more granular data as opposed to the current snapshot-aggregated telemetry.
The goal of this PR is to showcase a Proof-of-Concept of how this API will work while maintaining the main constraints we want to enforce:
To ensure those constraints this PR implements:
schema
that will be audited to ensure no PII-related data is collected [Trust]. The schema is used to validate the incoming events, so we'll discard anything that doesn't match.PluginScopedAPIs
. This prepends the caller's plugin name as an argument to the callee API so plugins cannot impersonate others to gain access to a larger allowance.telemetry
logger to silent by default in production ([Telemetry] Custom logger appender? #89839), and allows users to explicitly enable on demand by setting thetelemetry.logging
config according to the logging settings. This will log the events for users to be aware of what we collect about them [Transparency]TODOs
telemetry
README filetelemetry
settings asciidoc to explain about the new logging featuresChecklist
Delete any items that are not applicable to this PR.
For maintainers