-
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
[APM] adds telemetry to APM #25513
[APM] adds telemetry to APM #25513
Conversation
💚 Build Succeeded |
) | ||
} as ApmTelemetry | ||
); | ||
} |
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.
What do you think about this simplification?
const validAgentNames = agentNames.filter(
agentName => Object.values(AgentName).includes(agentName)
);
return {
has_any_services: validAgentNames.length > 0,
services_per_agent: _.countBy(validAgentNames)
};
It's not 100% equivalent to yours though since it omits the zero values. Not sure if that's a deal-breaker.
} catch (err) { | ||
return createApmTelementry(); | ||
} | ||
return apmTelemetrySavedObject.attributes; |
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.
You can avoid the let
like:
const savedObjectsClient = getSavedObjectsClient(server);
try {
const apmTelemetrySavedObject = await savedObjectsClient.get(
'apm-telemetry',
APM_TELEMETRY_DOC_ID
);
return apmTelemetrySavedObject.attributes;
} catch (err) {
return createApmTelementry();
}
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. How would I test this locally?
export function getSavedObjectsClient(server: Server): any { | ||
const { | ||
savedObjects: { SavedObjectsClient, getSavedObjectsRepository } | ||
} = server; |
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.
Personally I find this easier to read:
const { SavedObjectsClient, getSavedObjectsRepository } = server.savedObjects;
getSavedObjectsClient | ||
} from './apm_telemetry'; | ||
|
||
interface KibanaHapiServer extends Server { |
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.
Maybe add a TODO
that this should come from the platform
createApmTelementry( | ||
serviceBucketList.map(({ agent_name }) => agent_name as AgentName) | ||
) | ||
); |
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.
Maybe break it up for readability?
const apmTelemetry = createApmTelementry(serviceBucketList.map(({ agent_name }) => agent_name as AgentName))
storeApmTelemetry(server, apmTelemetry)
c142b35
to
348a8b6
Compare
I tested it locally by running elasticsearch, apm-server, and a number of services locally and pointed kibana to local ES. Then i would navigate to the APM landing page which shows all running services. finally i would |
💔 Build Failed |
💚 Build Succeeded |
* [APM] adds telemetry to APM * [APM] Code and readability improvements for APM Telemetry * [APM] fixed failing tests for apm-telemetry and service routes * [APM] fix lint issues for APM Telemetry
Summary
Fixes #24142 & #23318 by adding telemetry to APM
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers