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

[Synthetics] Add lightweight params support #148634

Merged
merged 47 commits into from
Mar 28, 2023

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Jan 10, 2023

Summary

Fixes #147467

Users will be able to specify params in following format in project monitors or UI

- type: http
  name: Admin Check
  url: ...
  username: ${admin_user:admin}
  password: ${admin_password}

@shahzad31 shahzad31 changed the title add lightweight params support [Synthetics] Add lightweight params support Jan 10, 2023
@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes v8.7.0 labels Feb 2, 2023
@shahzad31 shahzad31 marked this pull request as ready for review February 2, 2023 14:41
@shahzad31 shahzad31 requested a review from a team as a code owner February 2, 2023 14:41
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Feb 2, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@dominiqueclarke dominiqueclarke self-requested a review February 5, 2023 23:31
Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

My first basic test didn't seem to work for me.

I tried created an UI HTTP monitor.

I started out with setting the url to ${url:https://google.com}, before setting any params.

This worked as expected, as it fell back to https://google.com.

However, after adding a url param, the monitor did not update to use the param.

Upon further inspection, I noticed that the url param did not seem to be added at all. I removed the default and just used ${url}. When inspecting the config sent to the service, I noticed that the url is empty.

As a side note, we may want to prevent monitors before being saved with a configuration that results in an empty string for required values, such as url, hosts, and name for http, tcp, and icmp monitors. Otherwise, it could cause issues on the service side.

You can see my monitor config here.
Screen Shot 2023-02-05 at 8 00 01 PM

And you can see I have my param set here
Screen Shot 2023-02-05 at 8 00 13 PM

And you can see it's still running with the default here
Screen Shot 2023-02-05 at 8 01 55 PM

@shahzad31 shahzad31 requested review from a team as code owners February 6, 2023 12:14
@shahzad31 shahzad31 removed request for a team February 6, 2023 13:10
@dominiqueclarke dominiqueclarke self-requested a review February 6, 2023 15:11
Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

Doesn't work for http headers, where I would expect it to, for example to work with auth headers or other sensitive headers.

Screen Shot 2023-02-06 at 2 24 07 PM

@dominiqueclarke
Copy link
Contributor

@dominiqueclarke @shahzad31

Regarding: #148634 (review)

Doesn't work for http headers, where I would expect it to, for example to work with auth headers or other sensitive headers.

Is this being addressed as part of this PR? Would users be able to use global params in request headers for lightweight monitors?

It is being addressed in this PR.

@shahzad31 shahzad31 added v8.8.0 and removed v8.7.0 labels Mar 22, 2023
@dominiqueclarke dominiqueclarke self-requested a review March 24, 2023 15:40
});
it('plain string', () => {
const expected: ParsedVars = [{ content: 'string', type: 'nonvar' }];
const formatter = variableParser.parse('string', params);
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 understand why params is being passed to variableParser.parse.

I took a look at the defining for that function, it seems it takes input and an options object, but options doesn't seem to have anything to do with params.

Can you explain this for me?

I see params being used down below in replaceVarsWithParams(formatter, params) which makes perfect sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was just a leftover. i will clean it


return replaceVarsWithParams(parsedVars, params);
} catch (e) {
logger?.info(`error parsing vars for value ${JSON.stringify(value)}`);
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 this should maybe be logger?.warn and also I wonder if we want to log the actual error too. Since I ran into issues with my params, that would be helpful.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

When creating UI or project monitors in a non-default space, the "All spaces" params do not work. All spaces params do appear to work in the default space.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM. The issue with all the non-default space is being handled in a separate PR.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
synthetics 1357 1360 +3

Async chunks

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

id before after diff
synthetics 1.4MB 1.4MB +1.5KB
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3
synthetics 92 104 +12
total +15

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3
synthetics 98 110 +12
total +15

History

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

@shahzad31 shahzad31 merged commit 378c5c1 into elastic:main Mar 28, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 28, 2023
@shahzad31 shahzad31 deleted the lightweight-params branch June 23, 2023 13:53
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 release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Params support for lightweight monitors
6 participants