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

Add documentation clarification #3382

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

paulb-elastic
Copy link
Contributor

Following on from #3263, there are more places where this limitation was not reflected in the documentation. This is also true for not supporting ports in Project Monitors.

The docs have also been updated to clarify that monitors cannot run in headful mode, but always headless, and cannot be overridden with Playwright Options.

Copy link
Contributor

A documentation preview will be available soon:

Copy link
Contributor

mergify bot commented Nov 23, 2023

This pull request does not have a backport label. Could you fix it @paulb-elastic? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-/d./d is the label to automatically backport to the /d./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Nov 23, 2023
@paulb-elastic paulb-elastic added backport-8.9 Automated backport with mergify backport-8.8 Automated backport with mergify backport-8.10 Automated backport with mergify backport-8.11 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Nov 23, 2023
@paulb-elastic
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/docs

@paulb-elastic paulb-elastic marked this pull request as ready for review November 23, 2023 12:17
@paulb-elastic paulb-elastic requested a review from a team as a code owner November 23, 2023 12:17
@paulb-elastic
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/docs

@@ -77,6 +77,11 @@ Read more in <<synthetics-params-secrets>>.

For all available options, refer to the https://playwright.dev/docs/test-configuration[Playwright documentation].

[NOTE]
====
Synthetics will always run browser monitors in headless mode (it is not possible to configure them to run in headful mode, even if setting Playwright options).
Copy link
Contributor

@lucabelluccini lucabelluccini Nov 23, 2023

Choose a reason for hiding this comment

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

I would have said "even forcing headless:false via Playwright options, we do not support running browser monitors in headful mode".

Maybe, can share an hint of passing a user agent like here if really needed? https://github.com/elastic/synthetics/blob/d33d899bafa686da0b547afd2145db51b411ed1b/examples/todos/synthetics.config.ts#L12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucabelluccini I thought the confusion was originally because someone was specifically trying to run in headful mode - were they doing this because they wanted a non headless UA string then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not have the whole detail about it, we can check together in Slack

@@ -167,6 +167,11 @@ Use Playwright to simulate and validate user workflows including:

Visit the https://playwright.dev/docs[Playwright documentation] for information.

[NOTE]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing for me, I've used headful mode for debugging on synthetics cli, shouldn't this specifically refer to Elastic hosted synthetics service/heartbeat monitors instead?

Running headful locally used to be supported. IMO, it should still be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emilioalvap that's correct, it is still possible via the CLI, which is why I didn't add the note there

I also specifically mentioned ...run browser monitors... to try and distinguish this as the monitor (not running the journey locally for debugging purposes), but it sounds like this was still confusing. I didn't want to add a link here to the CLI saying it was still possible, as I thought it would get too confusing, but do you think that would help then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording on the CLI specific page is quite detailed, I'd replicate that here:

Do not attempt to run in headful mode when running through Elastic’s global managed testing infrastructure or Private Locations as this is not supported.

IMO, this page is more generic than the CLI one to simply state that headful mode is not supported, it might be confusing for users as well.

emilioalvap
emilioalvap previously approved these changes Nov 24, 2023
Copy link
Contributor

@emilioalvap emilioalvap left a comment

Choose a reason for hiding this comment

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

LGTM

@mdbirnstiehl mdbirnstiehl requested review from colleenmcginnis and removed request for mdbirnstiehl November 27, 2023 17:23
@colleenmcginnis
Copy link
Contributor

@elasticmachine, run elasticsearch-ci/docs

Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

Some questions and suggestions below.

Comment on lines 15 to +18
* *A full URL using the syntax `scheme://<host>:[port]`*, where:
** `scheme` is one of `tcp`, `plain`, `ssl` or `tls`. If `tcp` or `plain` is specified, Synthetics establishes a TCP connection even if the monitor is configured to use SSL. If `tls` or `ssl` is specified, Synthetics establishes an SSL connection. However, if the monitor is not configured to use SSL, the system defaults are used (currently not supported on Windows).
** `host` is the hostname.
** `port` is the port number. If `port` is missing in the URL, the <<monitor-tcp-ports,`ports`>> setting is required.
** `port` is the port number.
Copy link
Contributor

Choose a reason for hiding this comment

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

@paulb-elastic this is the change I pushed to get the build to succeed (this line was still linking to the ports option, which was removed below). Should the whole line be removed instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks @colleenmcginnis!!
I think this part of line 18 should still be there as it describes what the port is under the A full URL using the syntax section

[source,yaml]
----
hosts: ["localhost:8000"]
hosts: "localhost"
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed you removed "A plain host name, such as localhost, or an IP address." above. Should we also remove this example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - good catch, removing

* *A plain host name, such as `localhost`, or an IP address.*
If you specify this option, you must also specify a value for <<monitor-tcp-ports,`ports`>>. If the monitor is {heartbeat-ref}/configuration-ssl.html[configured to use SSL], Synthetics establishes an SSL/TLS-based connection. Otherwise, it establishes a plain TCP connection.
(<<synthetics-lightweight-data-string,string>>)
a| *Required*. The host to ping. The entries in the list can be:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
a| *Required*. The host to ping. The entries in the list can be:
a| *Required*. The host to ping. The value can be:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks

@colleenmcginnis colleenmcginnis removed the request for review from alaudazzi November 27, 2023 19:25
@paulb-elastic
Copy link
Contributor Author

Thanks for the feedback @colleenmcginnis, updated as per the comments

Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #118 succeeded 742a6d4ad31d42637cedba646a8cc0061a795fbe
  • 💔 Build #116 failed 742a6d4ad31d42637cedba646a8cc0061a795fbe
  • 💔 Build #115 failed 742a6d4ad31d42637cedba646a8cc0061a795fbe

@paulb-elastic paulb-elastic merged commit 3271331 into elastic:main Nov 28, 2023
3 checks passed
@paulb-elastic paulb-elastic deleted the add-doc-clarifications branch November 28, 2023 12:35
mergify bot pushed a commit that referenced this pull request Nov 28, 2023
* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)

# Conflicts:
#	docs/en/observability/synthetics-configuration.asciidoc
mergify bot pushed a commit that referenced this pull request Nov 28, 2023
* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)
mergify bot pushed a commit that referenced this pull request Nov 28, 2023
* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)
mergify bot pushed a commit that referenced this pull request Nov 28, 2023
* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)
colleenmcginnis pushed a commit to colleenmcginnis/observability-docs that referenced this pull request Dec 18, 2023
* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)

Co-authored-by: Paul Bianciardi <[email protected]>
colleenmcginnis pushed a commit to colleenmcginnis/observability-docs that referenced this pull request Dec 18, 2023
* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)

Co-authored-by: Paul Bianciardi <[email protected]>
colleenmcginnis pushed a commit to colleenmcginnis/observability-docs that referenced this pull request Dec 18, 2023
* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)

Co-authored-by: Paul Bianciardi <[email protected]>
colleenmcginnis added a commit to colleenmcginnis/observability-docs that referenced this pull request Dec 18, 2023
…#3384)

* Add documentation clarification (elastic#3382)

* Update how hosts and ports are configured
* Add that monitors cannot be run headful

(cherry picked from commit 3271331)

# Conflicts:
#	docs/en/observability/synthetics-configuration.asciidoc

* fix conflicts

---------

Co-authored-by: Paul Bianciardi <[email protected]>
Co-authored-by: Colleen McGinnis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.8 Automated backport with mergify backport-8.9 Automated backport with mergify backport-8.10 Automated backport with mergify backport-8.11 Automated backport with mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants