Skip to content

Log correct Kibana URL when TLS is enabled and log it only once. #20721

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 18, 2018
Merged

Log correct Kibana URL when TLS is enabled and log it only once. #20721

merged 9 commits into from
Jul 18, 2018

Conversation

@azasypkin
Copy link
Member

@azasypkin azasypkin commented Jul 12, 2018 •
edited
Loading

While testing #19261 I've noticed that with enabled TLS and without base path proxy Kibana always logs HTTP url. The reason is that now "legacy" Kibana doesn't know anything about TLS and new platform doesn't log full URL.

In this PR I log full correct Kibana URL from new platform only.

Broken by #18951.

$ yarn start --oss --server.ssl.enabled=true \
  --server.ssl.key=/projects/elastic/master/kibana/test/dev_certs/server.key \
  --server.ssl.certificate=/projects/elastic/master/kibana/test/dev_certs/server.crt \
  --no-base-path

Before:
11
After:
1

$ yarn start --oss --server.ssl.enabled=true \
  --server.ssl.key=/projects/elastic/master/kibana/test/dev_certs/server.key \
  --server.ssl.certificate=/projects/elastic/master/kibana/test/dev_certs/server.crt

Before:
21
After:
2

$ yarn start --oss --no-base-path

Before:
31
After:
3

$ yarn start --oss

Before:
41
After:
4

@azasypkin azasypkin added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform v6.4.0 bug Fixes for quality problems that affect the customer experience labels Jul 12, 2018
azasypkin
azasypkin commented Jul 12, 2018
View reviewed changes
src/core/server/http/http_service.ts
);
} else {
this.log.info(`registering route handler for [${router.path}]`);
this.log.debug(`registering route handler for [${router.path}]`);
Copy link
Member Author

@azasypkin azasypkin Jul 12, 2018

Choose a reason for hiding this comment

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

note: we're going to register lots of routes in the future and logging every one with info level is probably too much...

azasypkin
azasypkin commented Jul 12, 2018
View reviewed changes
src/core/server/legacy_compat/logging/appenders/legacy_appender.ts
record.timestamp
);
public append({ level, context, message, error, timestamp, meta = {} }: LogRecord) {
const tags = [level.id.toLowerCase(), ...context.split('.'), ...(meta.tags || [])];
Copy link
Member Author

@azasypkin azasypkin Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

note: we weren't passing meta to legacy platform previously since we didn't know whether we really need it, turned out we need custom tags like listening that can't be part of context.

@azasypkin azasypkin requested review from rhoboat and spalger July 12, 2018 13:27
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 12, 2018

💔 Build Failed

@azasypkin
Copy link
Member Author

azasypkin commented Jul 12, 2018

retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 12, 2018

💚 Build Succeeded

spalger
spalger approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@spalger spalger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, code looks good, seems to do the right thing when I run it locally. starting server :tada: is adorable but do you think we can toss it while you're in here?

@azasypkin
Copy link
Member Author

azasypkin commented Jul 13, 2018 •
edited
Loading

starting server :tada: is adorable but do you think we can toss it while you're in here?

Yeah. What if we keep it with debug level instead (like we do in stop method)? It's one of the core services and we'd probably like to know if it tried to start, but only during some sort of troubleshooting.

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 13, 2018

💚 Build Succeeded

@azasypkin
Copy link
Member Author

azasypkin commented Jul 17, 2018

ping @archanid :)

@azasypkin azasypkin requested review from mistic and removed request for rhoboat July 18, 2018 15:07
mistic
mistic approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

Ran it locally and all seems as expected. LGTM @azasypkin

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 18, 2018

💔 Build Failed

@azasypkin
Copy link
Member Author

azasypkin commented Jul 18, 2018

retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 18, 2018

💔 Build Failed

@azasypkin
Copy link
Member Author

azasypkin commented Jul 18, 2018

retest

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 18, 2018

💔 Build Failed

@azasypkin
Copy link
Member Author

azasypkin commented Jul 18, 2018

Okay, something weird is going on CI, I'll take a look later today or on Friday when I'm back from vacation. Should be the test that's too flaky :)

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 18, 2018

💔 Build Failed

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 18, 2018

💚 Build Succeeded

@azasypkin azasypkin merged commit 2a645d2 into elastic:master Jul 18, 2018
@azasypkin azasypkin deleted the issue-xxx-log-server-listening-once branch July 18, 2018 23:27
azasypkin added a commit to azasypkin/kibana that referenced this pull request Jul 18, 2018
@azasypkin
Log correct Kibana URL when TLS is enabled and log it only once. (ela…
6e3911c 
…stic#20721)
@azasypkin azasypkin mentioned this pull request Jul 18, 2018
Merged
@azasypkin
Copy link
Member Author

azasypkin commented Jul 19, 2018

6.x/6.4: 750b6b8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mistic mistic mistic approved these changes

+1 more reviewer

@spalger spalger spalger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backported bug Fixes for quality problems that affect the customer experience Feature:New Platform Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v6.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@azasypkin @elasticmachine @spalger @mistic