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

Use doc link service in more Stack Monitoring pages #89050

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Jan 22, 2021

Summary

Related to #88107 and #88920

This PR uses doc link service keywords instead of hard-coded URLs in Stack Monitoring, so that they're easier to maintain.

@lcawl lcawl force-pushed the monitor-doc-links2 branch from 457a393 to 58292d7 Compare February 1, 2021 21:14
@igoristic
Copy link
Contributor

@elasticmachine merge upstream

@lcawl lcawl marked this pull request as ready for review February 3, 2021 16:59
@lcawl lcawl requested a review from a team as a code owner February 3, 2021 16:59
@lcawl lcawl requested a review from a team February 3, 2021 16:59
@lcawl lcawl added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 Feature:Stack Monitoring labels Feb 3, 2021
@@ -193,8 +198,11 @@ export class DocLinksService {
alertsKibanaDiskThreshold: `${ELASTIC_WEBSITE_URL}guide/en/kibana/${DOC_LINK_VERSION}/kibana-alerts.html#kibana-alerts-disk-usage-threshold`,
alertsKibanaJvmThreshold: `${ELASTIC_WEBSITE_URL}guide/en/kibana/${DOC_LINK_VERSION}/kibana-alerts.html#kibana-alerts-jvm-memory-threshold`,
alertsKibanaMissingData: `${ELASTIC_WEBSITE_URL}guide/en/kibana/${DOC_LINK_VERSION}/kibana-alerts.html#kibana-alerts-missing-monitoring-data`,
metricbeatBlog: `${ELASTIC_WEBSITE_URL}blog/external-collection-for-elastic-stack-monitoring-is-now-available-via-metricbeat`,
Copy link
Member

Choose a reason for hiding this comment

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

nit: As this URL doesn't rely on ${DOC_LINK_VERSION}, and there might be more metricbeat blog links in the future, it might make more sense to keep the prior implementation where the consuming code is just using ELASTIC_WEBSITE_URL directly with the blog path. Or alternatively to export a ELASTIC_WEBSITE_BLOG_URL to be used with a corresponding path. But no strong feelings on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far I haven't found a lot of blog links, so I think this announcement was exceptional. If more of them get added in the future, I agree that we should consider having a different base url that all of them can share (though I'd still vote for having keywords in the shared file rather than having the blog URLs sprinkled across the code.. we don't test those so it's easy to lose track of them).

Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Tested most of the links, looks pretty good 👍

Good to merge once CI is 🟢

@lcawl
Copy link
Contributor Author

lcawl commented Feb 4, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
monitoring 800.2KB 798.2KB -2.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 465.6KB 466.5KB +904.0B

History

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

@lcawl lcawl merged commit 0ef276d into elastic:master Feb 4, 2021
@lcawl lcawl deleted the monitor-doc-links2 branch February 4, 2021 19:45
lcawl added a commit to lcawl/kibana that referenced this pull request Feb 4, 2021
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 4, 2021
* master: (244 commits)
  [maps] Top hits per entity--change to title to use recent, minor edits (elastic#89254)
  [DOCS] Update installation details (elastic#90354)
  RFC for automatically generated typescript API documentation for every plugins public services, types, and functionality (elastic#86704)
  Elastic Maps Server config is `host` not `hostname` (elastic#90234)
  Use doc link services in index pattern management (elastic#89937)
  [Fleet] Managed Agent Policy (elastic#88688)
  [Workplace Search] Fix Source Settings bug  (elastic#90242)
  [Enterprise Search] Refactor MockRouter test helper to not store payload (elastic#90206)
  Use doc link service in more Stack Monitoring pages (elastic#89050)
  [App Search] Relevance Tuning logic - actions and selectors only, no listeners (elastic#89313)
  Remove UI filters from UI (elastic#89793)
  Use newfeed.service config for all newsfeeds (elastic#90252)
  skip flaky suite (elastic#85086)
  Add readme to geo containment alert covering test alert setup (elastic#89625)
  [APM] Enabling yesterday option when 24 hours is selected (elastic#90017)
  Test user for maps tests under import geoJSON tests (elastic#86015)
  [Lens] Hide column in table (elastic#88680)
  [Security Solution][Detections] Reduce detection engine reliance on _source (elastic#89371)
  [Discover] Minor cleanup (elastic#90260)
  [Search Session][Management] Rename "cancel" button and delete "Reload" button (elastic#90015)
  ...
lcawl added a commit that referenced this pull request Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Stack Monitoring release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants