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

Debugging with apm - fixes and tutorial #127892

Merged
merged 15 commits into from
Mar 22, 2022
Merged

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Mar 16, 2022

Summary

Improve the APM-Kibana setup and documentation.

  • Use values from kibana.dev.yml instead of apm.dev.js on local environments
  • Omit the secretToken from the configurations sent to the client (APM RUM doesn't need it)
  • Read process.env.ELASTIC_APM_SERVER_URL in packages/kbn-apm-config-loader/src/config.ts for consistency
  • Added jest tests
  • Added instructions for enabling APM on a local dev environment.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

const configPath = getConfigPath();

// Pick up settings from dev.yml as well
return [configPath, configPath.replace('kibana.yml', 'kibana.dev.yml')];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If anyone has a better idea how to get configs from kibana.dev.yml in a package, I'm open to it!

import { getConfiguration, shouldInstrumentClient } from '@kbn/apm-config-loader';

const OMIT_APM_CONFIG: Array<keyof AgentConfigOptions> = ['secretToken'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potential simple solution for #118505

* Get the configuration from the apm.dev.js file, supersedes config
* from the --config file, disabled when running the distributable
*/
private getDevConfig(): AgentConfigOptions {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using kibana.dev.yml is a more standard and intuitive way.

Copy link
Contributor

Choose a reason for hiding this comment

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

We knew we'd be changing where we read the config from eventually, just didn't have support for config in kibana.dev.yml at the time the package was added.

@@ -136,6 +136,10 @@ export class ApmConfiguration {
config.serverUrl = process.env.ELASTIC_APM_SERVER_URL;
}

if (process.env.ELASTIC_APM_SECRET_TOKEN) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these environemnt values are also picked up automatically by apm-node-agent.
However, we should be consistent, so I added this one here (ELASTIC_APM_SERVER_URL was passed in but ELASTIC_APM_SECRET_TOKEN was fetched by the agent)

@lizozom lizozom marked this pull request as ready for review March 21, 2022 14:22
@lizozom lizozom requested review from a team and vigneshshanmugam as code owners March 21, 2022 14:22
@@ -24,7 +24,7 @@ const DEFAULT_CONFIG: AgentConfigOptions = {
globalLabels: {},
};

const CENTRALIZED_SERVICE_BASE_CONFIG: AgentConfigOptions | RUMAgentConfigOptions = {
export const CENTRALIZED_SERVICE_BASE_CONFIG: AgentConfigOptions | RUMAgentConfigOptions = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exported for tests

@lizozom lizozom added release_note:skip Skip the PR/issue when compiling release notes v8.2.0 v8.3.0 v8.1.2 DevDocs labels Mar 21, 2022
@lizozom lizozom self-assigned this Mar 21, 2022
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 on the APM side.

@stacey-gammon
Copy link
Contributor

I don't know what is going on with GitHub, but I can't seem to find the place to LGTM! Doc changes look good.

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

Found it. Doc changes LGTM

@lizozom
Copy link
Contributor Author

lizozom commented Mar 21, 2022

@elasticmachine merge upstream

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

LGTM,
Thanks for the tutorial and docs update.

* Get the configuration from the apm.dev.js file, supersedes config
* from the --config file, disabled when running the distributable
*/
private getDevConfig(): AgentConfigOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

We knew we'd be changing where we read the config from eventually, just didn't have support for config in kibana.dev.yml at the time the package was added.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Looks fine overall, I've suggested a few changes to the docs.

dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
dev_docs/tutorials/debugging.mdx Outdated Show resolved Hide resolved
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

I pointed out two new typos but apart from that the changes look good to me. Please feel free to merge without another review round after addressing the changes.


Frontend (APM RUM):
* `http-request`- tracks all outgoing API requests
* `page-load` - tracks the inidial loading time of kibana
Copy link
Member

Choose a reason for hiding this comment

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

Nit - typo: inidial -> initial

dev_docs/tutorials/debugging.mdx Show resolved Hide resolved
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @lizozom

@lizozom lizozom merged commit c97bfc8 into elastic:main Mar 22, 2022
@lizozom lizozom added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 22, 2022
kibanamachine pushed a commit that referenced this pull request Mar 22, 2022
* fixes + tutorial

* cors config

* omit secretToken so its not sent to FE
Add config tests

* lint

* empty

* swallow errors when parsing configs

* read config test adjustment

* apm docs review

* new line

* doc review

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit c97bfc8)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.3 The branch "8.3" is invalid or doesn't exist
8.1

Manual backport

To create the backport manually run:

node scripts/backport --pr 127892

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Mar 22, 2022
* fixes + tutorial

* cors config

* omit secretToken so its not sent to FE
Add config tests

* lint

* empty

* swallow errors when parsing configs

* read config test adjustment

* apm docs review

* new line

* doc review

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit c97bfc8)

Co-authored-by: Liza Katz <[email protected]>
@lizozom lizozom removed the v8.3.0 label Mar 22, 2022

1. Create a secondary monitoring deployment to collect APM data. The easiest option is to use [Elastic Cloud](https://cloud.elastic.co/deployments) to create a new deployment.
2. Open Kibana, go to `Integrations` and enable the Elastic APM integration.
3. Scroll down and copy the server URL and secret token. You may also find them in your cloud console under APM & Fleet.
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 attach some screenshots to help identify them in UI?


* ELASTIC_APM_ACTIVE
* ELASTIC_APM_SERVER_URL
* ELASTIC_APM_SECRET_TOKEN
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 an example? ELASTIC_APM_ACTIVE=true; yarn start;...?

const readYaml = (path: string) => {
try {
return safeLoad(readFileSync(path, 'utf8'));
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess safeLoad function shouldn't throw an exception :D

lizozom pushed a commit to lizozom/kibana that referenced this pull request Apr 6, 2022
* fixes + tutorial

* cors config

* omit secretToken so its not sent to FE
Add config tests

* lint

* empty

* swallow errors when parsing configs

* read config test adjustment

* apm docs review

* new line

* doc review

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit c97bfc8)

# Conflicts:
#	packages/kbn-apm-config-loader/src/config.test.ts
#	packages/kbn-apm-config-loader/src/config.ts
#	src/core/server/http_resources/get_apm_config.ts
lizozom pushed a commit that referenced this pull request Apr 6, 2022
* Debugging with apm - fixes and tutorial (#127892)

* fixes + tutorial

* cors config

* omit secretToken so its not sent to FE
Add config tests

* lint

* empty

* swallow errors when parsing configs

* read config test adjustment

* apm docs review

* new line

* doc review

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit c97bfc8)

# Conflicts:
#	packages/kbn-apm-config-loader/src/config.test.ts
#	packages/kbn-apm-config-loader/src/config.ts
#	src/core/server/http_resources/get_apm_config.ts

* fix merge
Mpdreamz added a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
This automatically sets up APM in any Elasticsearch instance that Kibana starts through @elastic/kbn-es. This includes integration and e2e tests.

It respects the same configuration as kibana and can be locally overridden in kibana.dev.yml as per elastic#127892.

It will bootstrap `secret_token` and `api_key` in the local Elasticsearch secret store but as this appears to be broken for now these are passed directly to Elasticsearch as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed DevDocs release_note:skip Skip the PR/issue when compiling release notes v7.17.3 v8.1.2 v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants