-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Flexible EBT Performance Metric Schema #136395
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.
Looks fine but I left a few comments.
src/core/utils/events.ts
Outdated
* Side Public License, v 1. | ||
*/ | ||
|
||
// TODO: replace with kibana-loaded |
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.
Leftover?
src/plugins/dashboard/public/application/embeddable/dashboard_container.tsx
Outdated
Show resolved
Hide resolved
… normalize-ebt-events
… normalize-ebt-events
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.
Echoing what we commented on Slack: I'm mostly concerned with the breaking changes we are introducing. Especially when we are still discussing if this is the event structure and any potential wrappers/API-helpers we may want to provide in the future.
While in phase 1 we don't need these API-helpers, we may introduce them in future phases to improve the Developer Experience, so, IMO, we should think about what event structure and implementation will give us the highest level of flexibility to adapt to new requirements as we find them in the journey.
test/analytics/tests/instrumented_events/from_the_browser/loaded_kibana.ts
Outdated
Show resolved
Hide resolved
@afharo I resovled both backward compatibility problems and used underscores to help the indexer index the event names correctly. |
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.
I did another pass after your most recent changes. I left a few comments but consider all of them only nits. The PR looks good me. Please feel free to address my nits but from my end it does not require an additional review cycle.
packages/core/analytics/core-analytics-browser-internal/src/analytics_service.test.ts
Outdated
Show resolved
Hide resolved
packages/core/analytics/core-analytics-browser-internal/src/analytics_service.ts
Outdated
Show resolved
Hide resolved
packages/core/analytics/core-analytics-server-internal/src/analytics_service.test.ts
Outdated
Show resolved
Hide resolved
packages/core/analytics/core-analytics-server-internal/src/analytics_service.ts
Outdated
Show resolved
Hide resolved
test/analytics/tests/instrumented_events/from_the_browser/loaded_kibana.ts
Outdated
Show resolved
Hide resolved
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.
Added some nits.
We'll discuss tomorrow in our meeting how to best approach the keyN/valueN
vs meta
😇
packages/core/analytics/core-analytics-browser-internal/src/analytics_service.test.ts
Outdated
Show resolved
Hide resolved
packages/core/analytics/core-analytics-server-internal/src/analytics_service.test.ts
Show resolved
Hide resolved
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.
LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
In order to simplify the consumption of performance telemetry, we’d like to standardize the shape of events. This requires considering the following aspects:
Balancing these requirements required certain compromises that led us to the proposed solution:
Performance metric events are registered once (on server and on client) using the
event_type: performance_metric
and a standardized base schema:event_type
, so that we can avoid registering it multiple times. This requires us to add a special standard field foreventName
.duration
for time based reports,status
,heapSize
,memorySize
, etc. Standardized fields will be mapped as is to the index, so they can be visualized.key1
to always report the loading time of a certain component that is always present on the page. Free fields will be mapped, if present, to the index, so they can be visualized. If a free field becomes popular, it can be promoted to a standardized field, freeing up the free field for other use.meta
object. However those won’t be automatically mapped to the index. Since the properties field is a Flattened Field, this means those fields will be searchable by default, but an additional field mapping would be required to visualize them.For example:
These events will then be mapped as implemented in https://github.com/elastic/telemetry/pull/1459