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

feat: create MetricsContext for ksql metrics reporters #5528

Merged
merged 3 commits into from
Jun 4, 2020
Merged

feat: create MetricsContext for ksql metrics reporters #5528

merged 3 commits into from
Jun 4, 2020

Conversation

stevenpyzhang
Copy link
Member

@stevenpyzhang stevenpyzhang commented Jun 2, 2020 •
edited
Loading

Description

Follow up to
https://github.com/apache/kafka/pull/8691/files

Testing done

Updated unit tests to pass

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@stevenpyzhang stevenpyzhang force-pushed the add-metrics-context-ksql branch 2 times, most recently from 7ad7489 to 81657ea Compare June 3, 2020 04:04
@stevenpyzhang stevenpyzhang marked this pull request as ready for review June 3, 2020 04:28
@stevenpyzhang stevenpyzhang requested a review from a team as a code owner June 3, 2020 04:28
@stevenpyzhang stevenpyzhang force-pushed the add-metrics-context-ksql branch 5 times, most recently from 0cf1ea2 to a74fcc7 Compare June 3, 2020 17:39
@stevenpyzhang stevenpyzhang force-pushed the add-metrics-context-ksql branch from a74fcc7 to 26fff48 Compare June 3, 2020 18:23
@stevenpyzhang stevenpyzhang requested a review from xiaodongdu June 3, 2020 18:23
xiaodongdu
xiaodongdu reviewed Jun 3, 2020
View reviewed changes
ksqldb-common/src/main/java/io/confluent/ksql/metrics/MetricCollectors.java Outdated
updatedProps.put(RESOURCE_LABEL_TYPE, KSQL_RESOURCE_TYPE);
updatedProps.put(RESOURCE_LABEL_KSQL_SERVICE_ID, ksqlServiceId);
updatedProps.put(RESOURCE_LABEL_CLUSTER_ID, ksqlServiceId);
updatedProps.put(RESOURCE_LABEL_KAFKA_CLUSTER_ID, kafkaClusterId);
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this. According to KIP-606, "kafka.cluster.id" is for kafka broker. For client of TelemetryReporter, you only need resource.cluster.id

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

xiaodongdu
xiaodongdu reviewed Jun 3, 2020
View reviewed changes
ksqldb-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java
return Collections.unmodifiableMap(props);
}

public Map<String, Object> getKsqlAdminClientConfigProps() {
return getConfigsFor(AdminClientConfig.configNames());
final Map<String, Object> map = new HashMap<>();
Copy link
Member

@xiaodongdu xiaodongdu Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If you want ksql cluster id and application propagate to AdminClient, you should add as part of admin client properties:
metrics.context.cluster.id, metrics.context.ksql.application.id

Because in KafkaAdminClient, we treat properties as metrics reporter properties if it starts with metrics.context: https://github.com/confluentinc/ce-kafka/blob/0f61da6b355f2f34e1b6d2f30072bb47f600bdae/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java#L495

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated it so that metrics.context.resource.cluster.id=<ksql-service-id> is being passed in for adminClient, producer, and streams

xiaodongdu
xiaodongdu reviewed Jun 3, 2020
View reviewed changes
ksqldb-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java Outdated
final Map<String, Object> map = new HashMap<>();
map.putAll(getConfigsFor(ProducerConfig.configNames()));

map.putAll(getConfigsForPrefix(REPORTER_CONFIGS_PREFIXES));
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as AdminClient

@stevenpyzhang stevenpyzhang force-pushed the add-metrics-context-ksql branch 3 times, most recently from 5d09d36 to 72e6413 Compare June 3, 2020 22:33
@stevenpyzhang
Copy link
Member Author

stevenpyzhang commented Jun 3, 2020 •
edited
Loading

Updated so that when KSQL is passing configs to consumer, producer, and AdminClient, then the configs should contain

metrics.context.resource.commit.id=<kafka-commit-id>
metrics.context.resource.version=<kafka-version>
metrics.context.resource.type=KSQL
metrics.context.resource.cluster.id=<ksql-service-id>

Streams created by KSQL will have an additional config

metrics.context.resource.application.id=<application-id>

KSQL Metrics Context

resource.commit.id=<ksql-commit-id>
resource.version=<ksql-version>
resource.type=KSQL
resource.cluster.id=<ksql-service-id>
resource.ksql.service.id=<ksql-service-id>

@stevenpyzhang stevenpyzhang force-pushed the add-metrics-context-ksql branch 4 times, most recently from 03f6fa6 to b80b081 Compare June 4, 2020 06:55
@jcrobak jcrobak requested a review from xiaodongdu June 4, 2020 13:11
@stevenpyzhang stevenpyzhang force-pushed the add-metrics-context-ksql branch from b80b081 to 9ff74dd Compare June 4, 2020 14:11
@stevenpyzhang stevenpyzhang force-pushed the add-metrics-context-ksql branch from 9ff74dd to 4190131 Compare June 4, 2020 14:53
agavra
agavra approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM - just making sure, if you don't specify any METRIC_REPORTER_CLASSES_CONFIG then the default behavior is to report nothing back to confluent, right?

@stevenpyzhang stevenpyzhang merged commit 50561a5 into confluentinc:master Jun 4, 2020
@stevenpyzhang
Copy link
Member Author

METRIC_REPORTER_CLASSES_CONFIG

LGTM - just making sure, if you don't specify any METRIC_REPORTER_CLASSES_CONFIG then the default behavior is to report nothing back to confluent, right?

Yes, the list returned when trying to get configuredInstances will be empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agavra agavra agavra approved these changes

@xiaodongdu xiaodongdu Awaiting requested review from xiaodongdu

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

Successfully merging this pull request may close these issues.
3 participants
@stevenpyzhang @agavra @xiaodongdu