Connect probers and notifiers by Channel #108

haoel · 2022-05-24T09:39:48Z

This PR introduces the channel to connect the probers and notifiers.

For more information, please take a look the discussion #82

Example:

if we have the following configuration

http: 
   - name: probe A
     channels : [ Chann_A, Chann_B]
shell:
   - name: probe B
     channels: [ Chann_C ]
notify:
   - discord: Discord
     channels: [Chann_A, Chann_C]
   - email: Gmail
     channels: [Chann_B]

Then, we will have the following diagram

┌───────┐          ┌─────────┐
│Probe B├─────────►│ Chann_C ├─────┐
└───────┘          └─────────┘     │
                                   │
                                   │
                   ┌─────────┐     │    ┌─────────┐
            ┌─────►│ Chann_A ├─────▼────► Discord │
            │      └─────────┘          └─────────┘
┌───────┐   │
│Probe A├───┤
└───────┘   │
            │      ┌─────────┐          ┌─────────┐
            └─────►│ Chann_B ├──────────►  Gmail  │
                   └─────────┘          └─────────┘

Test

I've done the following test:

Case 1: backward compatibility - NO channels defined for all probers and notifiers.
Case 2 : 1 prober -> 1 channel -> 1 notifier.
Case 3: 5 probers and 3 notifiers, define 3 channels.
Case 4: the channel hasn't probers. - everything's fine, but a warning message in the log
Case 5: the channel hasn't notifiers - everything's fine, but a warning message in the log.
Case 6: the probers and notifiers without channels that can be connected together.

proditis

LGTM but maybe wait for another reviewer as these changes where a bit too much for my Go knowledge, and i'm not sure i fully understood the code. Will do a build/run test shortly and update with any details I find.

Well done @haoel

README.md

proditis · 2022-05-25T06:49:50Z

global/global.go

@@ -59,6 +59,8 @@ const (
 	DefaultProbeInterval = time.Second * 60
 	// DefaultTimeOut is 30 seconds
 	DefaultTimeOut = time.Second * 30
+	// DefaultChannelName  is the default wide channel name
+	DefaultChannelName = "__EaseProbe_Channel__"


what will happen if i define this as a channel name or i define a probe to use it like
channels: [ __EaseProbe_Channel__ ] ?

this would be considered the default channel, it would be grouped with all of the probers and notifiers who have not defined the channel.

zhao-kun · 2022-05-25T07:31:15Z

What's the problem the PR will solve?

If we want to let users can config which notification can be applied to the prober, I don't think we should expose the channel concept to the end-user, because it's technical and not intuitive. Can we give the notification a name and let it be referred in one or more probers?

haoel · 2022-05-25T08:10:47Z

What's the problem the PR will solve?

If we want to let users can config which notification can be applied to the prober, I don't think we should expose the channel concept to the end-user, because it's technical and not intuitive. Can we give the notification a name and let it be referred in one or more probers?

The channel is not the Go concept, it's the channel literally.

To design like this, we have several considerations.

The requirement needs to group the notifiers, then the probers can just send the result to that group.
if we configure the reference of notifiers in the probe, this would couple the notify to probe. Although it works, but in some cases, it could bring complexity. for example, if we want to add or delete a notifier, we need to change many places.

Theoretically, this is an N x M relationship model, introducing a referring object would be the best practice.

You can take a look the discussion #82 to have more information.

Co-authored-by: Pantelis Roditis <[email protected]>

zhao-kun · 2022-05-25T08:20:55Z

What's the problem the PR will solve?
If we want to let users can config which notification can be applied to the prober, I don't think we should expose the channel concept to the end-user, because it's technical and not intuitive. Can we give the notification a name and let it be referred in one or more probers?

The channel is not the Go concept, it's the channel literally.

To design like this, we have several considerations.

The requirement needs to group the notifiers, and probers just send the result to that group.

if we configure the reference of notifiers in the probe, this would couple the notify to probe. it works, in some cases, it could bring complexity. for example, if we want to add or delete a notifier, we need to change many places.

Theoretically, this is an N x M relationship model, introducing a referring object would be the best practice.

You can take a look the discussion #82 to have more information.

OK, got it. It's better to relate the issue with the PR in its description. So reviewers can easy to know the background of the PR.

BTW, Why don't we choose the labels and selector mechanism, the mechanism was practiced and applied in the K8s, and cloud systems widely.

channel/channel.go

jordy1024

I mainly added some suggestions for improving code readability,for reference only.

cmd/easeprobe/report.go

cmd/easeprobe/prober.go

cmd/easeprobe/channel.go

cmd/easeprobe/main.go

cmd/easeprobe/notify.go

cmd/easeprobe/prober.go

jordy1024 · 2022-05-25T14:03:36Z

What's the problem the PR will solve?
If we want to let users can config which notification can be applied to the prober, I don't think we should expose the channel concept to the end-user, because it's technical and not intuitive. Can we give the notification a name and let it be referred in one or more probers?

The channel is not the Go concept, it's the channel literally.

To design like this, we have several considerations.

The requirement needs to group the notifiers, then the probers can just send the result to that group.

if we configure the reference of notifiers in the probe, this would couple the notify to probe. it works, in some cases, it could bring complexity. for example, if we want to add or delete a notifier, we need to change many places.

Theoretically, this is an N x M relationship model, introducing a referring object would be the best practice.

You can take a look the discussion #82 to have more information.

Sounds like a very flexible, extensible design. 👍🏻

haoel · 2022-05-26T02:02:37Z

BTW, Why don't we choose the labels and selector mechanism, the mechanism was practiced and applied in the K8s, and cloud systems widely.

It's easy to do the naming work

change the notify's channels to labels
change the probe's channels to selector

But I think the channels make more sense as the notification just like the TV Channel.

@proditis & @nullsimon, what do you think?

nullsimon · 2022-05-26T03:14:11Z

BTW, Why don't we choose the labels and selector mechanism, the mechanism was practiced and applied in the K8s, and cloud systems widely.

It's easy to do the naming work

change the notify's channels to labels

change the probe's channels to selector

But I think the channels make more sense as the notification just like the TV Channel.

@proditis & @nullsimon, what do you think?

Lables and selector are great and commonly used,it's a kv store extension,and channels is just a one feature of them. If we have more new features,it is extensive.

channel/channel.go

haoel · 2022-05-26T04:01:08Z

I just realize using labels and selector is not just naming works. it's different.

For channel, both probe and notify can define that, we just create the channel arbitrarily. So, the code is simpler.
For the labels and selector, only the labels can create the labels, and the selector won't and needs to check whether the label exists or not.

cc @zhao-kun @nullsimon @proditis

zhao-kun · 2022-05-26T04:03:46Z

BTW, Why don't we choose the labels and selector mechanism, the mechanism was practiced and applied in the K8s, and cloud systems widely.

It's easy to do the naming work

change the notify's channels to labels

change the probe's channels to selector

But I think the channels make more sense as the notification just like the TV Channel.

@proditis & @nullsimon, what do you think?

IMHO, the term channel represents different ways of sending notification, such as, we could treat email as a channel, SMS as another channel. The label helps us group different channels flexibly. So these terms would be much more intuitive to users and easy to use if we adopt them.

nullsimon · 2022-05-26T06:25:43Z

I just realize using labels and selector is not just naming works. it's different.

For channel, both probe and notify can define that, we just create the channel arbitrarily. So, the code is simpler.

For the labels and selector, only the labels can create the labels, and the selector won't and needs to check whether the label exists or not.

cc @zhao-kun @nullsimon @proditis

Yeah,channel is just one dimension,labels and selector become two dimensions. For channel to probers and notifies is that they have a common base metadata,while another become a pub-sub thing. The More extensive the more complex.

proditis · 2022-05-26T08:59:29Z

change the notify's channels to labels

change the probe's channels to selector

But I think the channels make more sense as the notification just like the TV Channel.

I like the common naming channels more also. i'm not familiar with k8d so i dont know the context the labels and selector keywords are used there.

I dont have strong arguments for or against the change, though, so i leave the final decision to you guys.

haoel · 2022-05-26T09:29:53Z

First of all, both of them have the same flexibility.

Secondly, I believe different people like different terms, the intuition is subjective.

Channel is business-specific, and Label/Selector is more general. If we want to make business sense, we should use the Channel, otherwise using Label/Selector.

So, we must make the decision here. As the owner and PR author, I will make the final decision - Using the Channel for this version ;-) as this is based on the original discussion, and it's easier for coding work.

Meanwhile, if we need the Label/Selector in the future, we still are open to doing that ...

proditis

LGTM

proditis · 2022-05-26T09:36:48Z

README.md

+> 
+> 1) If you don't define the Channel, the default channel will be used for these probers and notifiers. The default channel name is `__EaseProbe_Channel__` 
+> 
+> 2) Versions of EaseProbe prior to  v1.5.0, do not support the `channel` feature


I think we can remove the note from this spot and add it to the release notes of the next version.

This is the major important feature, I think it's better to mention it in the user manual. like other software.

proditis · 2022-05-26T09:38:15Z

README.md

+```YAML
+http: 
+   - name: probe A
+     channels : [ Dev_Channel, Manager_Channel ]


Do the channel names need to follow any particular rules?
Such as no space, no symbols etc?

looks like the limitation is just only for yaml file. If the chars can be accepted by yaml, then we can support that.

README.md

Co-authored-by: Pantelis Roditis <[email protected]>

haoel added 4 commits May 24, 2022 17:24

[WIP] prober -> channel -> notify

d6bebce

typo fix

97ce0b0

refactory the code

caa4e4c

write the README.md

a2cefd5

haoel changed the title ~~[WIP] Connect probers and notifiers by Channel~~ Connect probers and notifiers by Channel May 25, 2022

haoel requested review from zhao-kun and proditis May 25, 2022 05:49

haoel marked this pull request as ready for review May 25, 2022 05:49

proditis reviewed May 25, 2022

View reviewed changes

Update README.md

796bed5

Co-authored-by: Pantelis Roditis <[email protected]>

localvar reviewed May 25, 2022

View reviewed changes

channel/channel.go Outdated Show resolved Hide resolved

fix the concurrent bug

9c43eaf

jordy1024 reviewed May 25, 2022

View reviewed changes

haoel added 2 commits May 26, 2022 09:48

change the time.After() to time.NewTimer(), and remove the blank line

c8cd764

remove the blank line

f226310

haoel requested review from localvar and proditis May 26, 2022 01:52

localvar reviewed May 26, 2022

View reviewed changes

channel/channel.go Outdated Show resolved Hide resolved

remove the interface reference

7cd3718

haoel requested a review from localvar May 26, 2022 04:43

proditis approved these changes May 26, 2022

View reviewed changes

haoel and others added 4 commits May 26, 2022 18:34

rename the prober.go to probe.go

d3999b7

Update README.md

ea77034

Co-authored-by: Pantelis Roditis <[email protected]>

Update README.md

17f4071

Co-authored-by: Pantelis Roditis <[email protected]>

Update README.md

052c0c1

Co-authored-by: Pantelis Roditis <[email protected]>

localvar approved these changes May 27, 2022

View reviewed changes

haoel merged commit e6ab189 into megaease:main May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect probers and notifiers by Channel #108

Connect probers and notifiers by Channel #108

haoel commented May 24, 2022 •

edited

Loading

proditis left a comment

proditis May 25, 2022

haoel May 25, 2022

zhao-kun commented May 25, 2022 •

edited

Loading

haoel commented May 25, 2022 •

edited

Loading

zhao-kun commented May 25, 2022 •

edited

Loading

jordy1024 left a comment

jordy1024 commented May 25, 2022 •

edited by haoel

Loading

haoel commented May 26, 2022 •

edited

Loading

nullsimon commented May 26, 2022

haoel commented May 26, 2022

zhao-kun commented May 26, 2022 •

edited

Loading

nullsimon commented May 26, 2022

proditis commented May 26, 2022 •

edited

Loading

haoel commented May 26, 2022

proditis left a comment

proditis May 26, 2022

haoel May 27, 2022

proditis May 26, 2022

haoel May 26, 2022

Connect probers and notifiers by Channel #108

Connect probers and notifiers by Channel #108

Conversation

haoel commented May 24, 2022 • edited Loading

Example:

Test

proditis left a comment

Choose a reason for hiding this comment

proditis May 25, 2022

Choose a reason for hiding this comment

haoel May 25, 2022

Choose a reason for hiding this comment

zhao-kun commented May 25, 2022 • edited Loading

haoel commented May 25, 2022 • edited Loading

zhao-kun commented May 25, 2022 • edited Loading

jordy1024 left a comment

Choose a reason for hiding this comment

jordy1024 commented May 25, 2022 • edited by haoel Loading

haoel commented May 26, 2022 • edited Loading

nullsimon commented May 26, 2022

haoel commented May 26, 2022

zhao-kun commented May 26, 2022 • edited Loading

nullsimon commented May 26, 2022

proditis commented May 26, 2022 • edited Loading

haoel commented May 26, 2022

proditis left a comment

Choose a reason for hiding this comment

proditis May 26, 2022

Choose a reason for hiding this comment

haoel May 27, 2022

Choose a reason for hiding this comment

proditis May 26, 2022

Choose a reason for hiding this comment

haoel May 26, 2022

Choose a reason for hiding this comment

haoel commented May 24, 2022 •

edited

Loading

zhao-kun commented May 25, 2022 •

edited

Loading

haoel commented May 25, 2022 •

edited

Loading

zhao-kun commented May 25, 2022 •

edited

Loading

jordy1024 commented May 25, 2022 •

edited by haoel

Loading

haoel commented May 26, 2022 •

edited

Loading

zhao-kun commented May 26, 2022 •

edited

Loading

proditis commented May 26, 2022 •

edited

Loading