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.

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

Prepare APM agent configuration for production use #78697

Merged
merged 6 commits into from
Oct 5, 2020

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Sep 28, 2020

Summary

  • Allows APM agent to be used in distributable builds
  • Removes version from serviceName and uses serviceVersion tag instead.
  • Turns off HTTP header and body capture by default
  • Adds filter function to remove username PII info (populated by ES tracing automatically)

@joshdover joshdover changed the title Prepare apm for prod Prepare APM agent configuration for production use Oct 2, 2020
@joshdover joshdover force-pushed the prepare-apm-for-prod branch from 140e7db to 08aeaf6 Compare October 2, 2020 20:25
@joshdover joshdover requested review from vigneshshanmugam and a team October 2, 2020 20:26
@joshdover joshdover added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0 labels Oct 2, 2020
@joshdover joshdover marked this pull request as ready for review October 2, 2020 20:27
@joshdover joshdover requested a review from watson as a code owner October 2, 2020 20:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@joshdover joshdover requested a review from tylersmalley October 2, 2020 20:27
src/apm.js Outdated Show resolved Hide resolved
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

LGTM with the mentioned comment.

packages/kbn-apm-config-loader/src/config.ts Show resolved Hide resolved
@@ -30,8 +30,13 @@ const getDefaultConfig = (isDistributable: boolean): ApmAgentConfig => {
return {
active: false,
globalLabels: {},
centralConfig: false,
captureHeaders: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment explaining why it's disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am erring on the side of caution here and trying to avoid capturing any data that could be sensitive. We could be more selective about this and capture only headers we know to be safe (an allowlist of headers). WDYT?

@@ -30,8 +30,13 @@ const getDefaultConfig = (isDistributable: boolean): ApmAgentConfig => {
return {
active: false,
globalLabels: {},
centralConfig: false,
captureHeaders: false,
captureBody: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

1.in elatic-apm-node/index.d.ts:

type CaptureBody = 'off' | 'errors' | 'transactions' | 'all';
  1. is it possible to improve type safety of
    export type ApmAgentConfig = Record<string, any>;
    ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgayvallet I believe we were needing some type changes from APM agent for this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

AgentConfigOptions is not exported from @elastic/apm-rum, but we could declare a compatible interface, maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will handle in follow up. Should not affect 7.10 since we still need to add config-schema validation for 'productionizing' this feature.

src/apm.js Show resolved Hide resolved
*/
constructor(private readonly apmConfig?: ApmConfig) {
this.enabled = process.env.IS_KIBANA_DISTRIBUTABLE !== 'true' && apmConfig != null;
this.enabled = apmConfig != null && !!apmConfig.active;
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a hard time debugging the config. It's turned out we don't read values from kibana.dev.yml. It might be another source of confusion for developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's turned out we don't read values from kibana.dev.yml

I think there was some confusion on whether or not we still supported this. It appears we do. Can fix as a follow up so I can get this in for FF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened issue: #79490
Should be a pretty quick follow up

packages/kbn-apm-config-loader/src/config.ts Show resolved Hide resolved
@joshdover
Copy link
Contributor Author

Pushed updates that addressed comments here and also fixed the APM serviceName in development (was broken by #79379, resulting in the main server and basePath proxy server both reporting as the "kibana-proxy" service)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
core 661.2KB 661.2KB +23.0B

History

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

@joshdover joshdover merged commit c3076a8 into elastic:master Oct 5, 2020
@joshdover joshdover deleted the prepare-apm-for-prod branch October 5, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants