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

Send APM Server config to Kibana #5424

Merged
merged 14 commits into from
Jun 23, 2021

Conversation

stuartnelson3
Copy link
Contributor

@stuartnelson3 stuartnelson3 commented Jun 9, 2021

Motivation/summary

Send a sanitized and flattened version of the apm-server config to kibana on
startup.

Closes #5377

Checklist

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

Start up kibana, elasticsearch and apm-server. Verify that the apm-server
config is indexed in elasticsearch.

Related issues

Depends on elastic/kibana#100657

@stuartnelson3
Copy link
Contributor Author

stuartnelson3 commented Jun 9, 2021

I'm removing anything involving ESConfig or Kibana, is there anything else that should be removed? apm-server.ssl.keyphrase, instrumentation.api_key, and instrumentation.secret_token all look like candidates.

Question regarding the flattened format: Do we want to do any sort of lower camelcasing for the config values? Or just leave them with their default formatting, ie.

{
  "APIKeyConfig.Enabled": false,
  "APIKeyConfig.LimitPerMin": 100,
  "AgentConfigs": null,
  "Aggregation.ServiceDestinations.Enabled": true,
  "Aggregation.ServiceDestinations.Interval": 60000000000,
  "Aggregation.ServiceDestinations.MaxGroups": 10000,
  "Aggregation.Transactions.Enabled": true,
  "Aggregation.Transactions.HDRHistogramSignificantFigures": 2,
  "Aggregation.Transactions.Interval": 60000000000,
  "Aggregation.Transactions.MaxTransactionGroups": 10000,
  "AugmentEnabled": true,
  "DataStreams.Enabled": false,
  "DefaultServiceEnvironment": "",
  "Expvar.Enabled": false,
  "Expvar.URL": "/debug/vars",
  "Host": "localhost:8200",
  "Register.Ingest.Pipeline.Enabled": true,
  "Register.Ingest.Pipeline.Overwrite": false,
  "Register.Ingest.Pipeline.Path": "ingest/pipeline/definition.json",
  "ResponseHeaders": null,
  "RumConfig.AllowHeaders": [],
  "RumConfig.AllowOrigins": [
    "*"
  ],
  "RumConfig.AllowServiceNames": null,
  "RumConfig.Enabled": false,
  "RumConfig.EventRate.Limit": 300,
  "RumConfig.EventRate.LruSize": 1000,
  "RumConfig.ExcludeFromGrouping": "^/webpack",
  "RumConfig.LibraryPattern": "node_modules|bower_components|~",
  "RumConfig.ResponseHeaders": null,
  "RumConfig.SourceMapping.Cache.Expiration": 300000000000,
  "RumConfig.SourceMapping.Enabled": true,
  "RumConfig.SourceMapping.IndexPattern": "apm-*-sourcemap*",
  "SelfInstrumentation.APIKey": "",
  "SelfInstrumentation.Enabled": false,
  "SelfInstrumentation.Environment": "",
  "SelfInstrumentation.Hosts": null,
  "SelfInstrumentation.Profiling.CPU.Duration": 10000000000,
  "SelfInstrumentation.Profiling.CPU.Enabled": false,
  "SelfInstrumentation.Profiling.CPU.Interval": 60000000000,
  "SelfInstrumentation.Profiling.Heap.Enabled": false,
  "SelfInstrumentation.Profiling.Heap.Interval": 60000000000,
  "SelfInstrumentation.SecretToken": "",
  "ShutdownTimeout": 5000000000,
  "TLS": null,
  "WriteTimeout": 30000000000
}

@apmmachine
Copy link
Contributor

apmmachine commented Jun 9, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #5424 updated

  • Start Time: 2021-06-23T11:31:42.354+0000

  • Duration: 44 min 15 sec

  • Commit: 81d0be4

Test stats 🧪

Test Results
Failed 0
Passed 6165
Skipped 119
Total 6284

Trends 🧪

Image of Build Times

Image of Tests

@axw
Copy link
Member

axw commented Jun 10, 2021

I'm removing anything involving ESConfig or Kibana, is there anything else that should be removed? apm-server.ssl.keyphrase, instrumentation.api_key, and instrumentation.secret_token all look like candidates.

None of these can be set in ESS, but to be on the safe side I think it would be a good idea to remove:

  • apm-server.ssl.*
  • apm-server.instrumentation.*
  • *.elasticsearch.*
  • instrumentation.*

@simitt will that work?

Question regarding the flattened format: Do we want to do any sort of lower camelcasing for the config values? Or just leave them with their default formatting, ie.

I think we should be sending the libbeat config input, rather than the apm-server config into which it is unmarshaled. The raw libbeat config is accessible in

rawConfig *common.Config

@simitt
Copy link
Contributor

simitt commented Jun 10, 2021

None of these can be set in ESS, but to be on the safe side I think it would be a good idea to remove:

apm-server.ssl.*
apm-server.instrumentation.*
*.elasticsearch.*
instrumentation.*

Following ssl related settings need to be synced: apm-server.ssl.enabled, apm-server.ssl.certificate, apm-server.ssl.key. Removing the other settings is fine.

I think we should be sending the libbeat config input, rather than the apm-server config into which it is unmarshaled.

Why is that? On the Kibana side we need to define a list of supported config options (hardcoded).

@axw
Copy link
Member

axw commented Jun 10, 2021

Why is that? On the Kibana side we need to define a list of supported config options (hardcoded).

A couple of reasons:

  • If we refactor the config structs (e.g. rename fields), then the JSON will change. This should be stable for the rest of 7.x, right? We could add json:"..." struct tags, but we already have a defined stable config format so may as well use that.
  • The config that's being marshalled at the moment includes defaults, which I don't think should be transferred?

@simitt
Copy link
Contributor

simitt commented Jun 10, 2021

@axw good points; +1

kibana/send_config.go Outdated Show resolved Hide resolved
@ogupte
Copy link

ogupte commented Jun 21, 2021

In the Kibana PR, I'm defining only the configs which map to input vars in the package policy:

https://github.com/elastic/kibana/pull/102682/files#diff-ef993cd3669a97d649974bea6f79a1c2406abbf7ca7f1cc0d86cda4df7d2c5adR231-R254

{
  'apm-server.host': 'host',
  'apm-server.secret_token': 'secret_token',
  'apm-server.rum.enabled': 'enable_rum',
  'apm-server.default_service_environment': 'default_service_environment',
  'apm-server.rum.allow_service_names': 'rum_allow_service_names',
  'apm-server.rum.allow_origins': 'rum_allow_origins',
  'apm-server.rum.allow_headers': 'rum_allow_headers',
  'apm-server.rum.event_rate.limit': 'rum_event_rate_limit',
  'apm-server.rum.event_rate.lru_size': 'rum_event_rate_lru_size',
  'apm-server.max_event_size': 'max_event_bytes',
  'apm-server.capture_personal_data': 'capture_personal_data',
  'apm-server.max_header_size': 'max_header_bytes',
  'apm-server.idle_timeout': 'idle_timeout',
  'apm-server.read_timeout': 'read_timeout',
  'apm-server.shutdown_timeout': 'shutdown_timeout',
  'apm-server.write_timeout': 'write_timeout',
  'apm-server.max_connections': 'max_connections',
  'apm-server.expvar.enabled': 'expvar_enabled',
  'output.elasticsearch.ssl.enabled': 'tls_enabled',
  'output.elasticsearch.ssl.certificate': 'tls_certificate',
  'output.elasticsearch.ssl.key': 'tls_key',
  'output.elasticsearch.ssl.supported_protocols': 'tls_supported_protocols',
  'output.elasticsearch.ssl.cipher_suites': 'tls_cipher_suites',
  'output.elasticsearch.ssl.curve_types': 'tls_curve_types',
}

Anything that isn't included in this map will be displayed to the user as unsupported.

@stuartnelson3
Copy link
Contributor Author

@ogupte we've made a couple changes to the input vars but haven't pushed them to the package policy yet. There's one final change for 7.14 in #5410, and then we'll pushed an updated version.

The config should look like the example you posted in your comment, ie. with the full path for each config. The example I posted above was from only marshalling the things nested under apm-server, but the full raw config will be marshaled.

@stuartnelson3 stuartnelson3 marked this pull request as ready for review June 22, 2021 15:01
@stuartnelson3 stuartnelson3 requested a review from a team June 22, 2021 15:01
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

generally looks good, left minor comments

// TODO: What sort of response will we get?
if resp.StatusCode > http.StatusOK {
return fmt.Errorf("bad response %s", resp.Status)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe cover the scenario where Kibana < 7.14 and therefore doesn't support the endpoint. (log and stop retrying)

protocol: "https"
username: "elastic"
password: "changeme"
worker: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

instrumentation and kibana are not covered here; can you also cover some config option that doesn't start with apm-server to check it is sent.

@simitt
Copy link
Contributor

simitt commented Jun 23, 2021

I just realized that the config options only contain the apm-server.* options, and not the output.*, setup.*, etc related config options, and that the options are not prefixed with apm-server.* anymore at this point (for example apm-server.ssl is only ssl at the point of flattening).

Since only apm-server options are supported as input vars in the apm integration, I guess that is fair, but we need to add some note to the APM migration UI that none of the other config options is ever supported.

The config options are currently sent without the apm-server.* prefix. The flattenAndClean check assumes that the whole configuration (including output.*, instrumentation.*), etc. is there and that the server specific settings are prefixed with apm-server.* which is not the case. Please change the logic and the testcase accordingly. When sending the options to Kibana, the apm-server prefix should be added, to make it straight forward on the Kibana side.

@axw
Copy link
Member

axw commented Jun 23, 2021

If we need to, I believe it is still possible to get the parent config: cast the *common.Config to *ucfg.Config, and use the latter's Parent method: https://pkg.go.dev/github.com/elastic/go-ucfg#Config.Parent

This raw config only has apm-server values, but
casting and then accessing the parent has the full
paths for the received config.
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Apart from a minor comment on the testcase, changes LGTM now.

kibana/send_config_test.go Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2021

This pull request is now in conflicts. Could you fix it @stuartnelson3? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b apm-config-to-kibana upstream/apm-config-to-kibana
git merge upstream/master
git push upstream apm-config-to-kibana

@stuartnelson3 stuartnelson3 merged commit 1920f95 into elastic:master Jun 23, 2021
@stuartnelson3 stuartnelson3 deleted the apm-config-to-kibana branch June 23, 2021 12:17
mergify bot pushed a commit that referenced this pull request Jun 23, 2021
(cherry picked from commit 1920f95)

# Conflicts:
#	beater/beater.go
#	changelogs/head.asciidoc
simitt added a commit to simitt/apm-server that referenced this pull request Jun 28, 2021
axw pushed a commit that referenced this pull request Jun 29, 2021
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@simitt
Copy link
Contributor

simitt commented Jul 9, 2021

Tested this on an ECE instance.

mergify bot pushed a commit that referenced this pull request Jul 9, 2021
(cherry picked from commit 1920f95)

# Conflicts:
#	changelogs/head.asciidoc
#	kibana/send_config.go
#	kibana/send_config_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send APM Server configurations to Kibana APM endpoint on startup
5 participants