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

✨ feat: added kafka producer #3268

Merged
merged 19 commits into from
Jul 17, 2023

Conversation

mhkarimi1383
Copy link
Contributor

@mhkarimi1383 mhkarimi1383 commented Jun 14, 2023

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Being able to monitor kafka instances by producing a message to them

I'm using producer since when kafka goes down or nodes became unavailable the producers are the first and most affected parts of ecosystem

NOTE: I have not tested SSL and some of the SASL Mechanisms, they should work but it would be great if someone test that thing :)

Type of change

Please delete any options that are not relevant.

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image
image
image
image
image
image

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I only noticed that this is still a draft after reviewing this, sorry

In general, this monitor is well written, but also include screenshots of it working and how you tested this monitor. (are there things you did not test?)
Does this deal with #139 (comment) ? Is this a common problem with kafka or does this not need to be adressed? CC: @cruscio

src/pages/EditMonitor.vue Show resolved Hide resolved
src/pages/EditMonitor.vue Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
src/pages/EditMonitor.vue Outdated Show resolved Hide resolved
server/util-server.js Show resolved Hide resolved
server/util-server.js Outdated Show resolved Hide resolved
server/util-server.js Show resolved Hide resolved
@mhkarimi1383
Copy link
Contributor Author

I only noticed that this is still a draft after reviewing this, sorry

In general, this monitor is well written, but also include screenshots of it working and how you tested this monitor. (are there things you did not test?)
Does this deal with #139 (comment) ? Is this a common problem with kafka or does this not need to be adressed? CC: @cruscio

Thanks for your review, but this one is not self-tested completely

But in Kafka (specially in K8s) when using Bootstrap Server instead of broker addresses your producers will get messed up when a node goes down, I don't know if this problem if coming from clients or no, but we want to make sure that producers are safe.

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383 mhkarimi1383 force-pushed the feat-add-kafka-producer branch from b74258e to 1030117 Compare June 16, 2023 14:27
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383 mhkarimi1383 marked this pull request as ready for review June 16, 2023 14:40
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@CommanderStorm
Copy link
Collaborator

(I still would prefer to see HiddenInput instead of the regular input, but can accept that we only do this for the notificationproviders until now)

@louislam louislam added this to the 1.23.0 milestone Jun 26, 2023
zappityzap and others added 2 commits June 27, 2023 16:13
* Add nostr DM notification provider

* require crypto for node 18 compatibility

* remove whitespace

Co-authored-by: Frank Elsinga <frank@elsinga.de>

* move closer to where it is used

* simplify success or failure logic

* don't clobber the non-alert msg

* Update server/notification-providers/nostr.js

Co-authored-by: Frank Elsinga <frank@elsinga.de>

* polyfills required for node <= 18

* resolve linter warnings

* missing comma

---------

Co-authored-by: Frank Elsinga <frank@elsinga.de>
@louislam
Copy link
Owner

I added two brokers, one is working, another one is not working. The result is UP, wondering if it is correct.

image

@louislam louislam added the question Further information is requested label Jul 16, 2023
@mhkarimi1383
Copy link
Contributor Author

I added two brokers, one is working, another one is not working. The result is UP, wondering if it is correct.

image

Yes that's correct in kafka we are adding new multiple brokers to have high availability,
It will health check brokers

Also you can test that with one broker hostname that resolves to multiple brokers (In K8s Setup we have something called bootstrap server that acts like that) but when a node goes down our applications some times fail (Clients are bad or bootstrap server is returning bad addresses)

@mhkarimi1383
Copy link
Contributor Author

But I don't know how KafkaJS handles load balancing and health checking, I found nothing in their docs

@louislam louislam removed the question Further information is requested label Jul 17, 2023
@louislam louislam merged commit 278b88a into louislam:master Jul 17, 2023
@mhkarimi1383 mhkarimi1383 deleted the feat-add-kafka-producer branch July 17, 2023 12:02
@chakflying
Copy link
Collaborator

chakflying commented Jul 17, 2023

The alignment of the mechanism dropdown is broken. I think vue-multiselect isn't needed here.
image

This also isn't setting the description (<p class="url"> in Details.vue) in the details page. Not a hard requirement but would be great if it's added.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants