[Synthetics] Allow Synthetics global params to include dashes #178054

justinkambic · 2024-03-05T21:43:20Z

Summary

Includes - in the regex to filter param names.

Testing

Repro instructions

Testing this is straightforward. You can verify the issue in main or on a cloud instance. If you spin up an ESS that is the most straightforward way.

In Synthetics UI, go to Settings > Global Parameters.

Create a param with some dashes in the key, i.e. TEST-PARAMS.
After saving, create an HTTP monitor. In the create form, you can scroll down and expand Advanced options. Include a check for Request headers, specify a header name and for the value, reference your global param using Beats env syntax.

You can either use Test Now or wait for it to fail, and you'll see issues get logged similar to what is documented in the attached issue.

Testing the fix

I will create an ESS deployment from this PR that will allow you to follow the same steps above. You should be able to see your monitor either succeed (if the response headers you check for are actually there), or fail for legitimate reasons. But you should not see the error documented in the attached issue, with a message like io:job could not be initialized....

elasticmachine · 2024-03-05T21:43:23Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

apmmachine · 2024-03-05T21:43:35Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

justinkambic · 2024-03-05T21:52:44Z

/oblt-deploy

vigneshshanmugam · 2024-03-05T22:00:30Z

...s/observability_solution/synthetics/server/synthetics_service/formatters/formatting_utils.ts

@@ -51,7 +51,7 @@ export const replaceStringWithParams = (
 };

 export const hasNoParams = (strVal: string) => {
-  const shellParamsRegex = /\$\{[a-zA-Z_][a-zA-Z0-9_]*\}/g;
+  const shellParamsRegex = /\$\{[a-zA-Z_][a-zA-Z0-9_-]*\}/g;


nit: why not keep it simple as \w+|[-_]?

Are we missing any other substution other than _ and - for parameters. Are dots used in beats config? There seems to be :, . used as well - https://www.elastic.co/guide/en/beats/libbeat/current/config-gile-format-refs.html

@vigneshshanmugam looking at the docs it appears that ? is also possible.

@vigneshshanmugam the : case is already covered by what we have today, you can see a a unit test to this effect in the same file.

Regarding the ?, it pertains to giving a custom error message for beats to output. I don't know if this is a feature we support or not, but doesn't seem super important here.

\w+|[-_] is not a drop-in fix, several of our covered usages fail whereas what I already added works for all of them. Frankly, I do not enjoy fiddling with regex so I am inclined to keep it as it is, to my eye it doesn't really reduce the overall complexity of the code much. I'm happy to put in something cleaner if all the tests keep passing though 😃.

I did add a \. to the character class to indicate the intention that we will support the . character per the case you linked above; I also included a new unit test. LMK if you have any further thoughts, thanks for the feedback 👍.

case is already covered by what we have today,

That is for substituion I beleieve, what if we set the key to test:param in the global params? If it works all good.

I am not sure if params would end up including any characters other than - and _. Just wanted to call out. I am fine with either way. Thanks for taking a look.

That is for substituion I beleieve, what if we set the key to test:param in the global params?

I'll investigate this a bit more 👍. Although, if you choose that as your param name.. I also feel like that is a bad naming strategy.

vigneshshanmugam

LGTM

cauemarcondes

LGTM

...s/observability_solution/synthetics/server/synthetics_service/formatters/formatting_utils.ts

cauemarcondes · 2024-03-06T16:13:09Z

...s/observability_solution/synthetics/server/synthetics_service/formatters/formatting_utils.ts

@@ -50,9 +50,10 @@ export const replaceStringWithParams = (
  return value as string | null;
 };

+export const SHELL_PARAMS_REGEX = /\$\{[a-zA-Z_][a-zA-Z0-9\._\-?:]*\}/g;


Why did you export it?

In the moment thinking of unit testing use case where someone writing a test would want to import it.. but now that I'm typing it I agree it's not needed, as it's encapsulated in the function.

Remove unneeded export.

kibana-ci · 2024-03-07T23:11:59Z

💚 Build Succeeded

Buildkite Build
Commit: ac0f551

Metrics [docs]

✅ unchanged

History

💔 Build #196325 failed 8992a7a
💛 Build #196238 was flaky 2a02e32
💚 Build #196057 succeeded a78a8a5

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

cc @justinkambic

…c#178054) ## Summary Resolves elastic#178053. Includes `-` in the regex to filter param names. ## Testing ### Repro instructions Testing this is straightforward. You can verify the issue in `main` or on a cloud instance. If you spin up an ESS that is the most straightforward way. 1. In Synthetics UI, go to Settings > Global Parameters. <img width="831" alt="image" src="https://github.com/elastic/kibana/assets/18429259/f6dc78f6-6e0c-4b84-a114-9e88266327e9"> 2. Create a param with some dashes in the key, i.e. `TEST-PARAMS`. 3. After saving, create an HTTP monitor. In the create form, you can scroll down and expand `Advanced options`. Include a check for `Request headers`, specify a header name and for the value, reference your global param using [Beats env syntax](https://www.elastic.co/guide/en/beats/libbeat/current/config-file-format-env-vars.html#_examples). <img width="831" alt="image" src="https://github.com/elastic/kibana/assets/18429259/bcf3c495-1db8-499c-a394-781564744679"> You can either use Test Now or wait for it to fail, and you'll see issues get logged similar to what is documented in the attached issue. ### Testing the fix I will create an ESS deployment from this PR that will allow you to follow the same steps above. You should be able to see your monitor either succeed (if the response headers you check for are actually there), or fail for legitimate reasons. But you should not see the error documented in the attached issue, with a message like `io:job could not be initialized...`. (cherry picked from commit d69045b)

kibanamachine · 2024-03-07T23:16:14Z

💚 All backports created successfully

Status	Branch	Result
✅	8.13

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…178054) (#178268) # Backport This will backport the following commits from `main` to `8.13`: - [[Synthetics] Allow Synthetics global params to include dashes (#178054)](#178054)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Justin Kambic <jk@elastic.co>

…c#178054) ## Summary Resolves elastic#178053. Includes `-` in the regex to filter param names. ## Testing ### Repro instructions Testing this is straightforward. You can verify the issue in `main` or on a cloud instance. If you spin up an ESS that is the most straightforward way. 1. In Synthetics UI, go to Settings > Global Parameters. <img width="831" alt="image" src="https://github.com/elastic/kibana/assets/18429259/f6dc78f6-6e0c-4b84-a114-9e88266327e9"> 2. Create a param with some dashes in the key, i.e. `TEST-PARAMS`. 3. After saving, create an HTTP monitor. In the create form, you can scroll down and expand `Advanced options`. Include a check for `Request headers`, specify a header name and for the value, reference your global param using [Beats env syntax](https://www.elastic.co/guide/en/beats/libbeat/current/config-file-format-env-vars.html#_examples). <img width="831" alt="image" src="https://github.com/elastic/kibana/assets/18429259/bcf3c495-1db8-499c-a394-781564744679"> You can either use Test Now or wait for it to fail, and you'll see issues get logged similar to what is documented in the attached issue. ### Testing the fix I will create an ESS deployment from this PR that will allow you to follow the same steps above. You should be able to see your monitor either succeed (if the response headers you check for are actually there), or fail for legitimate reasons. But you should not see the error documented in the attached issue, with a message like `io:job could not be initialized...`.

justinkambic added bug Fixes for quality problems that affect the customer experience Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.13.0 v8.14.0 labels Mar 5, 2024

justinkambic self-assigned this Mar 5, 2024

justinkambic requested a review from a team as a code owner March 5, 2024 21:43

justinkambic added the release_note:fix label Mar 5, 2024

vigneshshanmugam reviewed Mar 5, 2024

View reviewed changes

justinkambic force-pushed the 178053/dashes-are-not-supported-in-global-params-keys branch from 57837c3 to a78a8a5 Compare March 5, 2024 22:34

vigneshshanmugam approved these changes Mar 6, 2024

View reviewed changes

cauemarcondes approved these changes Mar 6, 2024

View reviewed changes

...s/observability_solution/synthetics/server/synthetics_service/formatters/formatting_utils.ts Outdated Show resolved Hide resolved

justinkambic force-pushed the 178053/dashes-are-not-supported-in-global-params-keys branch from a78a8a5 to 2a02e32 Compare March 6, 2024 16:08

cauemarcondes reviewed Mar 6, 2024

View reviewed changes

justinkambic force-pushed the 178053/dashes-are-not-supported-in-global-params-keys branch from 2a02e32 to 8992a7a Compare March 7, 2024 03:12

justinkambic enabled auto-merge (squash) March 7, 2024 03:12

justinkambic added 3 commits March 7, 2024 17:03

Allow Synthetics global params to include dashes in the key.

1320a5f

Support . as well.

fb5f7ba

Extract regex const declaration from function.

ac0f551

Remove unneeded export.

justinkambic force-pushed the 178053/dashes-are-not-supported-in-global-params-keys branch from 8992a7a to ac0f551 Compare March 7, 2024 22:04

justinkambic merged commit d69045b into elastic:main Mar 7, 2024
18 checks passed

kibanamachine mentioned this pull request Mar 7, 2024

[8.13] [Synthetics] Allow Synthetics global params to include dashes (#178054) #178268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Synthetics] Allow Synthetics global params to include dashes #178054

[Synthetics] Allow Synthetics global params to include dashes #178054

justinkambic commented Mar 5, 2024 •

edited by kibanamachine

elasticmachine commented Mar 5, 2024

apmmachine commented Mar 5, 2024

justinkambic commented Mar 5, 2024

vigneshshanmugam Mar 5, 2024

justinkambic Mar 5, 2024

justinkambic Mar 5, 2024

vigneshshanmugam Mar 5, 2024

justinkambic Mar 6, 2024

vigneshshanmugam left a comment

cauemarcondes left a comment

cauemarcondes Mar 6, 2024

justinkambic Mar 6, 2024

kibana-ci commented Mar 7, 2024

kibanamachine commented Mar 7, 2024

[Synthetics] Allow Synthetics global params to include dashes #178054

[Synthetics] Allow Synthetics global params to include dashes #178054

Conversation

justinkambic commented Mar 5, 2024 • edited by kibanamachine

Summary

Testing

Repro instructions

Testing the fix

elasticmachine commented Mar 5, 2024

apmmachine commented Mar 5, 2024

🤖 GitHub comments

justinkambic commented Mar 5, 2024

vigneshshanmugam Mar 5, 2024

Choose a reason for hiding this comment

justinkambic Mar 5, 2024

Choose a reason for hiding this comment

justinkambic Mar 5, 2024

Choose a reason for hiding this comment

vigneshshanmugam Mar 5, 2024

Choose a reason for hiding this comment

justinkambic Mar 6, 2024

Choose a reason for hiding this comment

vigneshshanmugam left a comment

Choose a reason for hiding this comment

cauemarcondes left a comment

Choose a reason for hiding this comment

cauemarcondes Mar 6, 2024

Choose a reason for hiding this comment

justinkambic Mar 6, 2024

Choose a reason for hiding this comment

kibana-ci commented Mar 7, 2024

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Mar 7, 2024

💚 All backports created successfully

Questions ?

justinkambic commented Mar 5, 2024 •

edited by kibanamachine