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

[Stack Monitoring] Use server.publicBaseUrl in Alert links #131154

Merged

Conversation

miltonhultgren
Copy link
Contributor

Fixes #127126

Rather than reading from core.Http. getServerInfo which gives info about the internal host, we use the config setting server.publicBaseUrl which should be set to the public facing URL users reach Kibana on when generating the links in alerts.

@miltonhultgren miltonhultgren added release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Feature:Stack Monitoring v8.3.0 labels Apr 28, 2022
@miltonhultgren miltonhultgren requested a review from a team as a code owner April 28, 2022 13:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@@ -344,6 +344,9 @@ export class BaseRule {
if (ccs) {
globalState.push(`ccs:${ccs}`);
}
return `${Globals.app.url}/app/monitoring#/${link}?_g=(${globalState.toString()})`;

const baseUrl = Globals.app.url || '(server.publicBaseUrl)';
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 don't know how we should react if the setting isn't configured, the default is simply 'undefined'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use the serverInfo as a fallback, or make it a relative path URL.

Copy link
Member

Choose a reason for hiding this comment

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

We expose the kibana base path itself as an action context variable within alerting, and leave it "set" to undefined if the config is not set. It renders as the empty string. Customers can "optionally" add it via a section block like, so it won't render anything if it's undefined:

Note you can one-line this multi-line template.

{{#kibanaBaseUrl}}
  [head to Kibana]({{kibanaBaseUrl}})
{{/kibanaBaseUrl}}


const baseUrl = Globals.app.url || '(server.publicBaseUrl)';

return `${baseUrl}/app/monitoring#/${link}?_g=(${globalState.toString()})`;
Copy link
Member

Choose a reason for hiding this comment

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

Since the input may be coming from the user (cloud sets the kibanaBaseUrl and it's not overridable by customers), would be best to use something like a relative URL "joiner" (not sure what the current flavor would be). Reason is, the customer may or may not be adding an trailing / to the URL, and you don't want to generate ...//app/... (though that might well work in some envs)

I believe I've cheated and used path.join() for this as well, but ... using a url or URL or whatever is correct these days would be better.

Copy link
Contributor Author

@miltonhultgren miltonhultgren Apr 29, 2022

Choose a reason for hiding this comment

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

Screenshot 2022-04-29 at 14 33 41

The Kibana docs state this value cannot end in /, so this should be safe?
I tested a few different configuration and kept getting errors about this so Kibana wouldn't start.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, if that's the case, shouldn't need a special join!

@miltonhultgren
Copy link
Contributor Author

@elasticmachine merge upstream

@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

@weltenwort weltenwort self-requested a review May 2, 2022 09:24
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

the changes LGTM 👍

the overall way the state is serialized doesn't look as good, but it's beyond the scope of this PR 😬

@miltonhultgren miltonhultgren merged commit 2ee62c4 into elastic:main May 2, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 2, 2022
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
…31154)

* [Stack Monitoring] Use server.publicBaseUrl in Alert links (elastic#127126)

* Use empty string as fallback value

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Stack Monitoring release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Stack Monitoring] Use BasePath.publicBaseUrl for links generated by SM rules
6 participants