-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add consumer_lag in Kafka consumergroup metricset #14822
Merged
+125
−48
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
d9b5704
Add consumer_lag in Kafka consumergroup metricset
ChrsMark 558a39b
Fix sub and tests
ChrsMark 3f8a29e
Include consumer_lag in unit_test checks
ChrsMark 2eec175
Refactor methods to reuse struct fields
ChrsMark 5379558
Merge remote-tracking branch 'upstream/master' into add_consumer_lag
ChrsMark 85a29aa
Close client connection
ChrsMark 0f6a8ef
Store cluster client in Broker struct for reusability
ChrsMark 6500bf7
Update data.json
ChrsMark 7061004
Review fixes
ChrsMark 240c16e
Add changelog
ChrsMark File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Store cluster client in Broker struct for reusability
Signed-off-by: chrismark <chrismarkou92@gmail.com>
commit 0f6a8eff9a5347858dce1b7f9df2eb9b72617251
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -43,6 +43,7 @@ func Version(version string) kafka.Version { | |||||
type Broker struct { | ||||||
broker *sarama.Broker | ||||||
cfg *sarama.Config | ||||||
client sarama.Client | ||||||
|
||||||
advertisedAddr string | ||||||
id int32 | ||||||
|
@@ -96,6 +97,7 @@ func NewBroker(host string, settings BrokerSettings) *Broker { | |||||
return &Broker{ | ||||||
broker: sarama.NewBroker(host), | ||||||
cfg: cfg, | ||||||
client: nil, | ||||||
id: noID, | ||||||
matchID: settings.MatchID, | ||||||
} | ||||||
|
@@ -104,6 +106,7 @@ func NewBroker(host string, settings BrokerSettings) *Broker { | |||||
// Close the broker connection | ||||||
func (b *Broker) Close() error { | ||||||
closeBroker(b.broker) | ||||||
b.client.Close() | ||||||
return nil | ||||||
} | ||||||
|
||||||
|
@@ -134,6 +137,13 @@ func (b *Broker) Connect() error { | |||||
debugf("found matching broker %v with id %v", other.Addr(), other.ID()) | ||||||
b.id = other.ID() | ||||||
b.advertisedAddr = other.Addr() | ||||||
|
||||||
c, err := getClusteWideClient(b.Addr(), b.cfg) | ||||||
if err != nil { | ||||||
closeBroker(b.broker) | ||||||
return fmt.Errorf("Could not get cluster client for advertised broker with address %v", b.Addr()) | ||||||
} | ||||||
b.client = c | ||||||
return nil | ||||||
} | ||||||
|
||||||
|
@@ -272,15 +282,10 @@ func (b *Broker) FetchGroupOffsets(group string, partitions map[string][]int32) | |||||
|
||||||
// GetPartitionOffsetFromTheLeader fetches the OffsetNewest from the leader. | ||||||
func (b *Broker) GetPartitionOffsetFromTheLeader(topic string, partitionID int32) (int64, error) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. Use
Suggested change
|
||||||
client, err := sarama.NewClient([]string{b.Addr()}, b.cfg) | ||||||
if err != nil { | ||||||
return -1, err | ||||||
} | ||||||
offset, err := client.GetOffset(topic, partitionID, sarama.OffsetNewest) | ||||||
offset, err := b.client.GetOffset(topic, partitionID, sarama.OffsetNewest) | ||||||
if err != nil { | ||||||
return -1, err | ||||||
} | ||||||
defer client.Close() | ||||||
return offset, nil | ||||||
} | ||||||
|
||||||
|
@@ -538,6 +543,14 @@ func anyIPsMatch(as, bs []net.IP) bool { | |||||
return false | ||||||
} | ||||||
|
||||||
func getClusteWideClient(addr string, cfg *sarama.Config) (sarama.Client, error) { | ||||||
mtojek marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
client, err := sarama.NewClient([]string{addr}, cfg) | ||||||
if err != nil { | ||||||
return nil, err | ||||||
} | ||||||
return client, nil | ||||||
} | ||||||
|
||||||
func brokerAddresses(brokers []*sarama.Broker) []string { | ||||||
addresses := make([]string, len(brokers)) | ||||||
for i, b := range brokers { | ||||||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I was thinking that we could use this client for everything, but not, we may still need to fetch offsets from non-leader partition replicas for monitoring purpouses.
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.
Though we can also use
client.Leader(topic, partitionID)
for that.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.
+1 on revisiting this in a followup PR with refactoring purpose