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

Kafka connect #1324

Merged
merged 3 commits into from Jun 22, 2023
Merged

Kafka connect #1324

merged 3 commits into from Jun 22, 2023

Conversation

meiao
Copy link
Contributor

@meiao meiao commented Jun 16, 2023

Overview

Adds instrumentation for Kafka Connect.
There are 3 new instrumentation modules.
The first adds support for extracting metrics from Kafka Connect.
The other 2 add transaction tracing, one for versions 2.0.0 up to 3.2.3, and the other for versions 3.3.0 and later.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023

Codecov Report

Merging #1324 (e739a61) into main (34e4bc1) will increase coverage by 1.23%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1324      +/-   ##
============================================
+ Coverage     61.72%   62.96%   +1.23%     
- Complexity     8636     8826     +190     
============================================
  Files           839      836       -3     
  Lines         40162    40111      -51     
  Branches       6073     6069       -4     
============================================
+ Hits          24790    25254     +464     
+ Misses        12891    12342     -549     
- Partials       2481     2515      +34     

see 37 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@meiao meiao force-pushed the kafka-connect branch 4 times, most recently from f39fb49 to 36c54e3 Compare June 21, 2023 14:33
Map<String, Object> eventData = METRICS_AS_EVENTS ? new HashMap<>() : Collections.emptyMap();
for (final Map.Entry<String, KafkaMetric> metric : metrics.entrySet()) {
Object metricValue = metric.getValue().metricValue();
if (metricValue instanceof Double) {
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 use metricValue instanceof Number? There could be missing metrics that cast to an integer.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition you can then get your float value like this:
final float value = ((Number) metricValue).floatValue();

Copy link
Contributor Author

@meiao meiao Jun 22, 2023

Choose a reason for hiding this comment

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

This was copied from the original Kafka metrics instrumentation.
But that is a good change. I checked and apparently they do have integer metrics.

Looks like this traces back to the early versions of the metrics where it always returned doubles.

private ScheduledFuture<?> scheduledFuture;

// the metrics are shared between Workers. So only one reporter needs to be attached.
public static void initialize(ConnectMetrics.MetricGroup metricGroup) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I'd like to refactor some of the other Kafka metrics reporters in streams/clients to use this approach.

Copy link
Contributor

@jtduffy jtduffy left a comment

Choose a reason for hiding this comment

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

+1

@meiao meiao merged commit 17c9ce2 into main Jun 22, 2023
102 checks passed
@meiao meiao deleted the kafka-connect branch June 22, 2023 20:26
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.

None yet

4 participants