Skip to content

[Uptime] 7.8 docs#66118

Merged
justinkambic merged 15 commits intoelastic:7.8from
justinkambic:uptime_7.8-docs-images
May 26, 2020
Merged

[Uptime] 7.8 docs#66118
justinkambic merged 15 commits intoelastic:7.8from
justinkambic:uptime_7.8-docs-images

Conversation

@justinkambic
Copy link
Contributor

Summary

Adds docs to 7.8. We'll forward-port this change to master after merging.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@justinkambic justinkambic added v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v7.9.0 labels May 11, 2020
@justinkambic justinkambic self-assigned this May 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

Thanks so much for adding these. Looks like there's an extra image file named Screen Shot 2020-05-11 at 2.44.25 PM.png . Left some additional comments as well. Would be great to get @bmorelli25 's feedback as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our docs generally use the active voice ( I can't find a citation, but maybe @bmorelli25 has one?) . This could be written: "Use the certificates page to visualize all of your TLS certificates..."

Copy link
Member

Choose a reason for hiding this comment

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

The Elastic guide to writing style talks about active voice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please have a look at 24ff319 where I've tried to prefer active voice as much as I could. I also tried to simplify a large chunk of the copy I had added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Active voice: "Specify thresholds for all TLS certificates on the settings page"

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to be this descriptive, that part of the app is self explanatory. IMHO we shouldn't document the obvious. Would like @bmorelli25 's thoughts here.

Perhaps the content would be better if it described what the fields vs. documenting their existence. Rephrasing this as: "Use the certificate maximum age field to control the number of days before expiration Uptime should display the certificate as being in a warning state. Warnings for maximum certificate age can also be set here. Some user agents and browsers consider certificates invalid after a certain amount of time.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. But looking at the screenshot, I only see "Expiration threshold" and "Age limit" as options. The docs should match those names. I'd alter Andrew's recommendation to something like this:

The settings page also allows you to specify thresholds for displaying and alerting on TLS certificates. Use the expiration threshold to specify <>, and the age limit field to control the <>. Some user agents and browsers consider certificates invalid after a certain amount of time.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be moved up IMHO to make the importance more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be in the first paragraph

@bmorelli25 bmorelli25 self-requested a review May 12, 2020 15:58
@justinkambic justinkambic force-pushed the uptime_7.8-docs-images branch from fad4c40 to 2ecbbfd Compare May 12, 2020 17:51
@justinkambic
Copy link
Contributor Author

Looks like there's an extra image file named Screen Shot 2020-05-11 at 2.44.25 PM.png

This was supposed to be a draft but I guess I marked it as ready 😿

I left that as a placeholder to remind me to re-take that screenshot after #66135 was merged. I have subsequently added commits that fix this.

Comment on lines +37 to +38
For example, you may want to make sure that none of your organization's TLS certificates have been
valid for longer than one year. This is becoming a common security requirement. Modifying the `Age limit`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example, you may want to make sure that none of your organization's TLS certificates have been
valid for longer than one year. This is becoming a common security requirement. Modifying the `Age limit`
For example, a common security requirement is to make sure that none of your organization's TLS certificates have been valid for longer than one year. Modifying the `Age limit`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 4a63705.

@justinkambic
Copy link
Contributor Author

What about something simpler

@bmorelli25 I updated the copy in 141c28e. Thanks!

Feel free to comment on any more of the copy I've added, these recommendations are super helpful.

@@ -0,0 +1,26 @@
[role="xpack"]
Copy link
Member

Choose a reason for hiding this comment

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

When I view the documentation preview for this page, I'm presented with two images without any context: http://kibana_66118.docs-preview.app.elstc.co/guide/en/kibana/7.8/uptime-alerting.html

I think we should move the paragraphs (context) above each of the screenshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this in f0a0de9.

Comment on lines +4 to +5
== Uptime alerting

Copy link
Member

Choose a reason for hiding this comment

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

Should we add an alerting overview here? Something like:

The Uptime app integrates with Kibana's {kibana-ref}/alerting-getting-started.html[alerting and actions] feature. It provides a set of built-in actions and Uptime specific threshold alerts for you to use
and enables central management of all alerts from <<management,Kibana Management>>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 0495751.

@justinkambic justinkambic requested a review from andrewvc May 13, 2020 13:41
@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@EamonnTP
Copy link

Hi @justinkambic Would it be possible to merge this PR before Wednesday? Next week, as part of the Observability docs restructuring, I plan to move the Uptime documentation in the Kibana reference to the Uptime guide and it would be great to include this new content. Thanks!

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@justinkambic justinkambic merged commit f0765f3 into elastic:7.8 May 26, 2020
justinkambic added a commit to justinkambic/kibana that referenced this pull request May 26, 2020
* Refresh existing docs images.

* Add tls-related images.

* Add certificates page to docs.

* Update settings page with explanation of cert threshold fields.

* Add placeholder image of updated settings page. Will need finalized version of this after we merge updated defaults for age limit threshold.

* Add new alerting docs page.

* Refresh settings page image.

* Implement feedback and re-write/simplify some sections.

* Clean up words.

* Implement feedback on sentence structure.

* Improve copy on alerting page.

* Improve copy on settings page.

* Update layout on alerting page.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
justinkambic added a commit that referenced this pull request May 26, 2020
* Refresh existing docs images.

* Add tls-related images.

* Add certificates page to docs.

* Update settings page with explanation of cert threshold fields.

* Add placeholder image of updated settings page. Will need finalized version of this after we merge updated defaults for age limit threshold.

* Add new alerting docs page.

* Refresh settings page image.

* Implement feedback and re-write/simplify some sections.

* Clean up words.

* Implement feedback on sentence structure.

* Improve copy on alerting page.

* Improve copy on settings page.

* Update layout on alerting page.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.8.0 v7.9.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants