-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[synthrace] Update entities schema #184019
base: main
Are you sure you want to change the base?
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
💔 Build Failed
Failed CI StepsTest Failures
Metrics [docs]Canvas Sharable Runtime
HistoryTo update your PR or re-run it, just comment with: |
@@ -75,19 +76,27 @@ function getAssetsFilterTransform() { | |||
}); | |||
} | |||
|
|||
function getMergeAssetsTransform() { | |||
const mergedDocuments: Record<string, AssetDocument> = {}; | |||
function getMergeEntitesTransform() { |
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: typo in spelling
|
||
avg({ key }: { key: string }) { | ||
const averageMap = new Map(); | ||
this.trackedValues.forEach((values, key) => { |
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: same name variable collision for key. Also in forEach in the rate function
* Side Public License, v 1. | ||
*/ | ||
|
||
class PivotTransform { |
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.
Given we are working with Streams here, the usage of name Transform seem misguiding. wdyt ? its actually a factory no ?
'entity.metric': { | ||
latency: createPivotTransform(), | ||
failedTransactionRate: createPivotTransform(), | ||
throughput: createPivotTransform(), |
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 don't see throughput using any of the record, avg or rate functions. Do you really want it to be a createPivotTransform
'entity.data_stream.type': ['logs'], | ||
'entity.firstSeen': firstSeen, | ||
'entity.metric': { | ||
logRatePerMinute: createPivotTransform(), |
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.
same as throughput
from the other aggregator, logRatePerMinute is not using any of the functions from this function, does it needs to be set here like this ?
@@ -45,6 +45,9 @@ export function createServiceMetricsAggregator(flushInterval: string) { | |||
sum: 0, | |||
value_count: 0, | |||
}, | |||
'data_stream.type': 'metrics', |
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.
Any specific reason to add these 3 values to all the APM aggregators ?
Concern i see is these 3 values together represent the actual DataStream Name where documents get indexed. And we have the data stream name logic defined in the routing transform here which is different.
packages/kbn-apm-synthtrace/src/lib/apm/client/apm_synthtrace_es_client/get_routing_transform.ts
@@ -33,4 +33,4 @@ export { generateLongId, generateShortId } from './src/lib/utils/generate_id'; | |||
export { appendHash, hashKeysOf } from './src/lib/utils/hash'; | |||
export type { ESDocumentWithOperation, SynthtraceESAction, SynthtraceGenerator } from './src/types'; | |||
export { log, type LogDocument } from './src/lib/logs'; | |||
export { type AssetDocument } from './src/lib/assets'; | |||
export { type EntityDocument } from './src/lib/assets'; |
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
export { type EntityDocument } from './src/lib/assets'; | |
export type { EntityDocument } from './src/lib/assets'; |
@@ -7,6 +7,6 @@ | |||
*/ |
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.
Should the file also be renamed?
|
||
export const createLogsAssetsAggregator = createAssetsAggregatorFactory<LogDocument>(); | ||
export const createLogsAssetsAggregator = createEntitiesAggregatorFactory<LogDocument>(); |
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.
Do we need to rename this const too?
export const createLogsAssetsAggregator = createEntitiesAggregatorFactory<LogDocument>(); | |
export const createLogsEntityAggregator = createEntitiesAggregatorFactory<LogDocument>(); |
@@ -7,6 +7,6 @@ | |||
*/ |
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.
Should the file also be renamed?
const logLevel = event['log.level']!; | ||
|
||
// @ts-expect-error | ||
entity['entity.metric'].logErrorRate.record({ |
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.
Suggestion to remove these ts-expect-error
entity['entity.metric'].logErrorRate.record({ | |
(entity['entity.metric.logErrorRate'] ?? {}) as PivotTransform).logErrorRate.record({ |
const tracesGen = createGeneratorFromArray(traces); | ||
const tracesGenAssets = createGeneratorFromArray(traces); | ||
|
||
return [ | ||
withClient( | ||
assetsEsClient, | ||
entitiesEsClient, | ||
logger.perf('generating_assets_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.
should we change this one too?
logger.perf('generating_assets_events', () => | |
logger.perf('generating_entities_events', () => |
@@ -15,6 +15,7 @@ import { | |||
log, |
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.
Should we rename the scenario too?
@@ -211,13 +214,13 @@ const scenario: Scenario<ApmFields> = async (runOptions) => { | |||
const logsGen = createGeneratorFromArray(logsValuesArray); | |||
const logsGenAssets = createGeneratorFromArray(logsValuesArray); |
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.
const logsGenAssets = createGeneratorFromArray(logsValuesArray); | |
const logsGenEntities = createGeneratorFromArray(logsValuesArray); |
@@ -211,13 +214,13 @@ const scenario: Scenario<ApmFields> = async (runOptions) => { | |||
const logsGen = createGeneratorFromArray(logsValuesArray); | |||
const logsGenAssets = createGeneratorFromArray(logsValuesArray); | |||
|
|||
const traces = instances.flatMap((instance) => instanceSpans(instance)); | |||
const traces = instances.flatMap((instance, index) => instanceSpans(instance, index)); | |||
const tracesGen = createGeneratorFromArray(traces); | |||
const tracesGenAssets = createGeneratorFromArray(traces); |
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.
const tracesGenAssets = createGeneratorFromArray(traces); | |
const tracesGenEntities = createGeneratorFromArray(traces); |
entitiesSynthtraceClient.index( | ||
Readable.from(Array.from(logsGenAssets).concat(Array.from(tracesGenAssets))) | ||
), | ||
logSynthtraceClient.index(logsGen), |
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.
Should we rename these too?
entitiesSynthtraceClient.index( | |
Readable.from(Array.from(logsGenAssets).concat(Array.from(tracesGenAssets))) | |
), | |
logSynthtraceClient.index(logsGen), | |
entitiesSynthtraceClient.index( | |
Readable.from(Array.from(logsGenEntities).concat(Array.from(tracesGenEntites))) | |
), | |
logSynthtraceClient.index(logsGen), |
Summary
How to test
Run
node scripts/synthtrace.js traces_logs_assets.ts --logLevel debug --clean
Dev tools
Produces