-
Notifications
You must be signed in to change notification settings - Fork 0
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
[DPE-6254] Update README.md #2
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks! Although I recommend getting a review from @izmalk too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just small comments, but I agree, let's also get this reviewed by @izmalk
juju relate grafana-agent kafka-benchmark | ||
``` | ||
|
||
The benchmark data will be collected every 10s and sent to prometheus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo could we add the Contributing and Licence section, as in other readme? https://github.com/canonical/kafka-operator/blob/main/README.md#contributing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, can you check?
[DPE-4514] Add opensearch-benchmark-operator
ba51196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
Left a few suggestions to improve.
Terminology notes (Apache Kafka
) should be considered as blocking.
LICENSE
Outdated
@@ -186,7 +187,7 @@ | |||
same "printed page" as the copyright notice for easier | |||
identification within third-party archives. | |||
|
|||
Copyright [yyyy] [name of copyright owner] | |||
Copyright 2024 Canonical Ltd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Shouldn't we use 2025 here already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to 2025
README.md
Outdated
|
||
### COS integration | ||
|
||
Relate the kafka benchmark with a [`grafana-agent` operator](https://charmhub.io/grafana-agent). For more details on how to configure COS and its agents, check [the upstream documentation](https://charmhub.io/grafana-agent/docs/using). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: Terminology fixes
Relate the kafka benchmark with a [`grafana-agent` operator](https://charmhub.io/grafana-agent). For more details on how to configure COS and its agents, check [the upstream documentation](https://charmhub.io/grafana-agent/docs/using). | |
Relate the Apache Kafka benchmark with a [`grafana-agent` operator](https://charmhub.io/grafana-agent). For more details on how to deploy and configure COS and its agents, check [the upstream documentation](https://charmhub.io/grafana-agent/docs/using). |
Question: Is that link sufficient for deploying COS and grafana-agent
? Wouldn't a link to the Apache Kafka monitoring How to guide better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but this question above has been left unanswered. Please make sure the link to the Grafana agent is the one we want here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@izmalk the link to grafana-agent is correct. RE:the steps to set up COS, I am moving to the Apache Kafka guide as you've referenced.
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
Co-authored-by: Vladimir Izmalkov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question seems to be missing an answer.
README.md
Outdated
|
||
### COS integration | ||
|
||
Relate the Apache Kafka benchmark with a [`grafana-agent` operator](https://charmhub.io/grafana-agent). For more details on how to deploy and configure COS and its agents, check [the upstream documentation](https://charmhub.io/grafana-agent/docs/using). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeating the question that might have been overlooked during the first review fix.
Question: Wouldn't it be better to provide the link to Charmed Apache Kafka How-to guide regarding monitoring with COS for more details?
Sorry if it's intentional, I just didn't find an answer in the comments and suspected the question might have been overlooked.
Updates the README with new instructions.