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 #1206: Do not require a "friendly name" #1231

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Computroniks
Copy link
Contributor

@Computroniks Computroniks commented Jan 28, 2022

Description

When creating or updating a monitor, a friendly name will no longer be required for specific monitor types. If it is not specified it will be set either the hostname or URL. In cases where both the URL and hostname are set (possible in instances where monitor type has been changed), the hostname has priority over the URL and the friendly name will be set to the hostname.

The array friendlyNameRequiredOptions contains all monitor types that still require a friendly name because they don't have either a URL or hostname field. For any monitor type specified in this array, the friendly name field will still be required.

Monitor types that no longer require a friendly name:

  • HTTP(s)
  • TCP Port
  • Ping
  • HTTP(s) - Keyword
  • DNS
  • Steam Game Server

Closes #1206

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@Computroniks
Copy link
Contributor Author

When I save the friendly name, it can either be "" or null. By using "" it would prevent issues with this line

return m1.name.localeCompare(m2.name);

but it would also show as an empty string in the prometheus metrics. For some reason it doesn't feel quite right just to leave it as an empty string in the metrics. Theoretically the empty string could just be changed to null when returning the prometheus metrics but this seems a bit messy. Any thoughts on what would be best here?

@Computroniks Computroniks changed the title Add #1206: Do not require a "friendly name" [WIP] Add #1206: Do not require a "friendly name" Jan 29, 2022
@karelkryda
Copy link
Contributor

karelkryda commented Jan 29, 2022

When I save the friendly name, it can either be "" or null. By using "" it would prevent issues with this line

return m1.name.localeCompare(m2.name);

but it would also show as an empty string in the prometheus metrics. For some reason it doesn't feel quite right just to leave it as an empty string in the metrics. Theoretically the empty string could just be changed to null when returning the prometheus metrics but this seems a bit messy. Any thoughts on what would be best here?

@Computroniks, If the monitor name is empty, simply set the monitor name to its URL. So the only thing you need to change is the way the data is stored in the database.

Ie. if the monitor name is empty => monitor.name = monitor.url ... this is the best solution in my opinion.

<input id="name" v-model="monitor.name" type="text" class="form-control" required>

Here set it to not required if "monitor.type === 'http' || monitor.type === 'keyword' "

async submit() {

Here you will check if it's empty and set it to it's URL

this.monitor = res.monitor;

And here check if URL and name are same (also if monitor.type === 'http' || monitor.type === 'keyword') and if so, just set name to ""

@Computroniks
Copy link
Contributor Author

@karelkryda I did consider that here:

Another way that this feature could be implemented is when a monitor is created, the url is copied into the name field either client or server side. The issue I see with this is if the URL were to change, it could get messy trying to work out if the friendly name should change as well.

#1206 (comment)

Whilst it doesn't seem like such of a big deal if someone changes the url and then has to change the friendly name as well, it seems a bit like only half a job to just copy it over to the name field.

@karelkryda
Copy link
Contributor

@Computroniks I don't see a problem with that.
As I say, if the monitor name and url match when the monitor is edited, reset the monitor name (set the name to empty), and if the monitor name is not entered during editing, set the monitor name to match the monitor url when saving.

@Computroniks
Copy link
Contributor Author

Ahh yes, of course. If I just implement a check to see if the URL and friendly name match when a user edits the monitor, I can then work out if the name should change as well. Not sure why I didn't think of that before. Thanks

@karelkryda

This comment was marked as resolved.

When adding or updating a monitor, if a friendly name is not specified,
the URL or hostname will be used instead. Note: the hostname has
priority over the URL so if both are set (possible in instances where
monitor type has been changed), the friendly name will be set to the
hostname.

Signed-off-by: Computroniks <[email protected]>
@Computroniks Computroniks changed the title [WIP] Add #1206: Do not require a "friendly name" Add #1206: Do not require a "friendly name" Jan 29, 2022
@Computroniks Computroniks marked this pull request as ready for review January 29, 2022 18:56
karelkryda

This comment was marked as resolved.

@Computroniks Computroniks force-pushed the #1206-no-friendly-name branch from f1b899b to 3e36d41 Compare January 29, 2022 23:25
Signed-off-by: Computroniks <[email protected]>

Co-authored-by: Adam Stachowicz <[email protected]>
@Computroniks

This comment was marked as resolved.

@gaby

This comment was marked as resolved.

@Computroniks

This comment was marked as resolved.

@Computroniks

This comment was marked as resolved.

@Computroniks
Copy link
Contributor Author

Computroniks commented Jun 11, 2022

Anything I can do to help get this merged?

@Computroniks

This comment was marked as resolved.

@Computroniks

This comment was marked as resolved.

@Computroniks Computroniks requested a review from Saibamen October 12, 2022 21:23
@Computroniks Computroniks requested review from karelkryda and Saibamen and removed request for Saibamen and karelkryda October 12, 2022 21:23
@Computroniks
Copy link
Contributor Author

@louislam Sorry to ping you, is there anything stopping this PR getting merged or that needs changing?

@louislam louislam added this to the 1.20.0 milestone Jan 10, 2023
…iendly-name

# Conflicts:
#	src/pages/EditMonitor.vue
@louislam
Copy link
Owner

Just testing it with different monitor types, found out that the friendly name sometimes do not follow the current url.

image

This video may help.
https://user-images.githubusercontent.com/1336778/213697855-7e083688-5b61-4d79-9114-a26c6745d50c.mp4

@Computroniks
Copy link
Contributor Author

This is probably because the docker monitor was added after this PR was first created and I haven't updated it to fix. I will get on that

@louislam louislam added the question Further information is requested label Jan 26, 2023
@louislam louislam modified the milestones: 1.20.0, Pending Feb 13, 2023
@Computroniks
Copy link
Contributor Author

Computroniks commented Feb 14, 2023

@louislam this PR should have now been updated to match all of the monitor types added recently. I have also modified the logic for selecting the field to use for the friendly name so that we don't end up with the issue where we are using a http monitor but the tcp hostname is used.

I also added a custom case for TCP monitors to include the port number being monitored.

@Computroniks Computroniks requested a review from karelkryda August 6, 2023 12:59
@Computroniks

This comment has been minimized.

Signed-off-by: Matthew Nickson <[email protected]>
Saibamen

This comment was marked as resolved.

Copy link
Contributor

@karelkryda karelkryda left a comment

Choose a reason for hiding this comment

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

LGTM :)

@Computroniks
Copy link
Contributor Author

@louislam This has been open for 2 years, are there any plans of merging it?

@CommanderStorm CommanderStorm added the pr:needs review this PR needs a review by maintainers or other community members label May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:resolve-merge-conflict pr:needs review this PR needs a review by maintainers or other community members question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not require a "friendly name"
6 participants