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.

Sign up for GitHub

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

Jump to bottom

Added bytesIn/OutPerSec params for clusterMetrics object #56

Merged
merged 12 commits into from
Jun 17, 2020
Merged

Added bytesIn/OutPerSec params for clusterMetrics object #56

merged 12 commits into from
Jun 17, 2020

Conversation

yazebochan
Copy link
Contributor

No description provided.

germanosin
germanosin suggested changes Jun 5, 2020
View reviewed changes
kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/util/ClusterUtil.java Outdated Show resolved Hide resolved
kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/util/ClusterUtil.java Outdated
public static Map<String, String> getJmxTrafficMetrics(String jmxUrl, String metricName) {
Map<String, String> result = new HashMap<>();
try {
JMXServiceURL url = new JMXServiceURL(jmxUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's bad idea to create connection each time, please use connection pool

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java Outdated
// TODO: fill bytes in/out metrics
Map<String, String> bytesInPerSec;
Map<String, String> bytesOutPerSec;
var jmx = kafkaJmxDto.getJmxParams().stream().filter(j -> j.getBrokerId() == c.id()).findFirst().orElseThrow();
Copy link
Contributor

Choose a reason for hiding this comment

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

You should get broker addresses from kafka cluster and use preconfigured jmx port

kafka-ui-api/src/main/resources/application-local.yml Outdated
@@ -1,4 +1,12 @@
kafka:
jmxParams:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's configure only jmx port per cluster and discovery brokers from cluster

@germanosin
Copy link
Contributor

  • Please check that you sum metric on cluster level
  • Please use BigDecimal for values

germanosin
germanosin suggested changes Jun 9, 2020
View reviewed changes
kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/config/ClustersProperties.java Outdated
@@ -19,5 +19,7 @@
String name;
String bootstrapServers;
String zookeeper;
int jmxPort;
String jmxHost;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/config/KafkaJmxProperties.java Outdated
List<Jmx> jmxParams = new ArrayList<>();

@Data
public static class Jmx {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be configured only on cluster level

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/model/InternalClusterMetrics.java Outdated
//TODO: find way to fill
private final int bytesInPerSec;
private final int bytesOutPerSec;
private final Map<String, String> bytesInPerSec;
Copy link
Contributor

Choose a reason for hiding this comment

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

BigDecimal

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/util/JmxClusterUtil.java Outdated

private static final List<String> attrNames = Arrays.asList("OneMinuteRate", "FiveMinuteRate", "FifteenMinuteRate");

private static KeyedObjectPool pool = new GenericKeyedObjectPool(new JmxPoolFactory());
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to bean

Copy link
Contributor

Choose a reason for hiding this comment

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

Configure min/max connections

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/util/JmxClusterUtil.java Outdated
Map<String, String> result = new HashMap<>();
JMXConnector srv = null;
try {
srv = (JMXConnector) pool.borrowObject(jmxUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

try with resources

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java Outdated
public Mono<ExtendedAdminClient> getOrCreateAdminClient(KafkaCluster cluster) {
Consumer<Long, String> kek = createConsumer();
Copy link
Contributor

Choose a reason for hiding this comment

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

debug?

kafka-ui-api/src/main/resources/application-local.yml
@@ -4,14 +4,23 @@ kafka:
name: local
bootstrapServers: localhost:29091
zookeeper: localhost:2183
jmxPort: 9997
Copy link
Contributor

Choose a reason for hiding this comment

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

only port

kafka-ui-contract/src/main/resources/swagger/kafka-ui-api.yaml
@@ -303,9 +303,13 @@ components:
topicCount:
type: integer
bytesInPerSec:
type: integer
type: object
Copy link
Contributor

Choose a reason for hiding this comment

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

number

kafka-ui-contract/src/main/resources/swagger/kafka-ui-api.yaml
bytesOutPerSec:
type: integer
type: object
Copy link
Contributor

Choose a reason for hiding this comment

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

number

Copy link
Contributor

Choose a reason for hiding this comment

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

why not just a number, why object?

kafka-ui-api/pom.xml Outdated
@@ -85,6 +85,12 @@
<artifactId>reactor-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-dbcp2</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

commons-pool?

germanosin
germanosin suggested changes Jun 11, 2020
View reviewed changes
kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/util/ClusterUtil.java Outdated
import reactor.core.publisher.Mono;

import javax.management.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this one is used here

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java Outdated
InternalClusterMetrics.InternalClusterMetricsBuilder metricsBuilder = InternalClusterMetrics.builder();
metricsBuilder.brokerCount(brokers.size()).activeControllers(c != null ? 1 : 0);
// TODO: fill bytes in/out metrics
Map<String, BigDecimal> bytesInPerSec;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you initialize it before method invocation?

@yazebochan yazebochan requested a review from apetrovs June 11, 2020 11:01
@apetrovs apetrovs self-requested a review June 17, 2020 10:57
@yazebochan yazebochan merged commit 32a09fa into master Jun 17, 2020
@yazebochan yazebochan deleted the backend-traffic-metrics branch June 17, 2020 11:44
javalover123 pushed a commit to javalover123/kafka-ui that referenced this pull request Dec 7, 2022
@yazebochan
Added bytesIn/OutPerSec params for clusterMetrics object (provectus#56)
b1fcbdb 
* Added bytesIn/OutPerSec params for clusterMetrics object

* Removed redundant todos, cleaned imports

* Jmx connections moved to pool, methods moved to separate classes

* Added pool handling and returning methods

* Fix after previous PR comments - fixed result map, configured pool, removed redundant methods and code

* Removed redundant imports and empty initialization

* Removed fill method

* Closing connection replaced to destroyObject method

* Try catch block while returning object to pool was fixed

* Removed redundant logs and try catch

Co-authored-by: Roman Nedzvetskiy <roman@Romans-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@germanosin germanosin germanosin requested changes

@apetrovs apetrovs apetrovs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yazebochan @germanosin @apetrovs