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: add consumer group id tag as global metric tag #161

Merged
merged 8 commits into from
Mar 24, 2022
Merged

feat: add consumer group id tag as global metric tag #161

merged 8 commits into from
Mar 24, 2022

Conversation

fzrvic
Copy link
Contributor

@fzrvic fzrvic commented Mar 22, 2022

No description provided.

@fzrvic fzrvic requested a review from lavkesh March 22, 2022 04:11
lavkesh
lavkesh reviewed Mar 22, 2022
View reviewed changes
src/main/java/io/odpf/firehose/metrics/Instrumentation.java Outdated
import static io.odpf.firehose.metrics.Metrics.SINK_RESPONSE_TIME_MILLISECONDS;
import static io.odpf.firehose.metrics.Metrics.SOURCE_KAFKA_MESSAGES_FILTER_TOTAL;
import static io.odpf.firehose.metrics.Metrics.SOURCE_KAFKA_PULL_BATCH_SIZE_TOTAL;
import static io.odpf.firehose.metrics.Metrics.*;
Copy link
Member

Choose a reason for hiding this comment

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

don't put asterisk. It will fail the checkstyle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes added

lavkesh
lavkesh reviewed Mar 22, 2022
View reviewed changes
src/main/java/io/odpf/firehose/consumer/kafka/FirehoseKafkaConsumer.java Outdated
import static io.odpf.firehose.metrics.Metrics.FAILURE_TAG;
import static io.odpf.firehose.metrics.Metrics.SOURCE_KAFKA_MESSAGES_COMMIT_TOTAL;
import static io.odpf.firehose.metrics.Metrics.SUCCESS_TAG;
import static io.odpf.firehose.metrics.Metrics.*;
Copy link
Member

Choose a reason for hiding this comment

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

don't put asterisk. It will fail the checkstyle..expand it like it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        <module name="AvoidStarImport">
          <property name="allowStaticMemberImports" value="true"/>
        </module>

it passed the checkstyle because currently the star (.*) import is currently allowed for static import
should we changes the checkstyle config too ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. that's fine.
but you don't have to edit current checkstyle or the code.

Copy link
Member

Choose a reason for hiding this comment

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

you can add your import there, don't delete the previous lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes added

lavkesh
lavkesh reviewed Mar 22, 2022
View reviewed changes
src/main/java/io/odpf/firehose/metrics/Instrumentation.java Outdated
@@ -199,6 +186,12 @@ public void captureGlobalMessageMetrics(Metrics.MessageScope scope, int counter)
statsDReporter.captureCount(GLOBAL_MESSAGES_TOTAL, counter, String.format(MESSAGE_SCOPE_TAG, scope.name()));
}

public void captureGlobalMessageMetrics(Metrics.MessageScope scope, int counter, String... tags) {
Copy link
Member

Choose a reason for hiding this comment

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

we can edit the previous method rather than adding a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@lavkesh
Copy link
Member

lavkesh commented Mar 22, 2022

@fzrvic Instead of adding consumer_group tag into only one metric, let's think if other metrics can also be benefited from this tag. We can add there too.

lavkesh
lavkesh reviewed Mar 23, 2022
View reviewed changes
config/checkstyle/checkstyle.xml Outdated
<module name="AvoidStarImport">
<property name="allowStaticMemberImports" value="true"/>
</module>
<module name="AvoidStarImport"/>
Copy link
Member

@lavkesh lavkesh Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

could we keep this?

Copy link
Member

Choose a reason for hiding this comment

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

@fzrvic please revert this and changes in the previous files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted

lavkesh
lavkesh reviewed Mar 23, 2022
View reviewed changes
src/main/java/io/odpf/firehose/metrics/Instrumentation.java Outdated
@@ -187,19 +188,21 @@ public void captureErrorMetrics(ErrorType errorType) {

// =================== Retry and DLQ Telemetry ======================

public void captureMessageMetrics(String metric, MessageType type, ErrorType errorType, int counter) {
public void captureMessageMetrics(String metric, Metrics.MessageType type, ErrorType errorType, int counter) {
Copy link
Member

Choose a reason for hiding this comment

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

why this line is changed?

lavkesh
lavkesh reviewed Mar 23, 2022
View reviewed changes
src/main/java/io/odpf/firehose/metrics/Instrumentation.java Outdated
}

public void captureMessageMetrics(String metric, MessageType type, int counter) {
public void captureMessageMetrics(String metric, Metrics.MessageType type, int counter) {
Copy link
Member

Choose a reason for hiding this comment

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

revert back this line

lavkesh
lavkesh reviewed Mar 23, 2022
View reviewed changes
config/checkstyle/checkstyle.xml Outdated
@@ -83,7 +83,7 @@
<!-- Checks for imports -->
<!-- See http://checkstyle.sf.net/config_import.html -->
<module name="AvoidStarImport">
<property name="allowStaticMemberImports" value="true"/>
<property name="allowStaticMemberImports" value="true"/>
Copy link
Member

Choose a reason for hiding this comment

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

there is a space.

lavkesh
lavkesh reviewed Mar 23, 2022
View reviewed changes
src/main/java/io/odpf/firehose/sink/prometheus/builder/HeaderBuilder.java Outdated
@@ -5,7 +5,11 @@
import java.util.Map;
import java.util.stream.Collectors;

import static io.odpf.firehose.sink.prometheus.PromSinkConstants.*;
Copy link
Member

Choose a reason for hiding this comment

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

there should not be any change in these files

@fzrvic fzrvic changed the title feat: add consumer group id tag to global message total metric feat: add consumer group id tag as global metric tag Mar 24, 2022
@lavkesh lavkesh merged commit 75fb130 into main Mar 24, 2022
@lavkesh lavkesh deleted the consumer-group-id-tag branch March 24, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavkesh lavkesh lavkesh left review comments

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

Successfully merging this pull request may close these issues.
2 participants
@fzrvic @lavkesh