Skip to content
This repository has been archived by the owner on Mar 17, 2024. It is now read-only.

Ability to extend and configure desired sink to report lag metrics, adding support to push lag metrics into InfluxDB as well #157

Merged
merged 8 commits into from
Dec 17, 2020
Merged

Ability to extend and configure desired sink to report lag metrics, adding support to push lag metrics into InfluxDB as well #157

merged 8 commits into from
Dec 17, 2020

Conversation

hariprasad-k
Copy link
Contributor

@hariprasad-k hariprasad-k commented Sep 14, 2020 •
edited
Loading

Follow up of #130 after rebasing with latest release

Note: This retains currently supported Prometheus as the default sink, if it's not configured.

@hariprasad-k
Copy link
Contributor Author

Fixed #129

@hariprasad-k
Copy link
Contributor Author

@seglo As per #130 (comment), we have rebased with the latest changes and added support to push metrics into InfluxDB too. Please let me know your thoughts.

@hariprasad-k hariprasad-k changed the title Changes to support pushing lag metrics into InfluxDB Ability to extend and configure desired sink to report lag metrics, adding support to push lag metrics into InfluxDB as well Sep 14, 2020
seglo
seglo reviewed Sep 15, 2020
View reviewed changes
Copy link
Owner

@seglo seglo left a comment

Choose a reason for hiding this comment

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

Thanks for continuing this effort. I like the sink configuration re-factor. I'm not very familiar with influxdb so I'll need to rely on you to fill in the gaps.

Could you look into adding some tests?

build.sbt Outdated Show resolved Hide resolved
src/main/scala/com/lightbend/kafkalagexporter/AppConfig.scala Show resolved Hide resolved
src/main/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSinkConfig.scala Outdated Show resolved Hide resolved
src/main/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSink.scala Show resolved Hide resolved
src/main/scala/com/lightbend/kafkalagexporter/MetricsReporter.scala Outdated
@@ -24,7 +24,7 @@ object MetricsReporter {
case (context, Stop(sender)) =>
Behaviors.stopped { () =>
metricsSink.stop()
context.log.info("Gracefully stopped Prometheus metrics endpoint HTTP server")
context.log.info("Gracefully stopped metrics sink")
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be useful here if each sink had a name so it could be logged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each sink has a name assigned now and stored in their config object. However, this is not accessible within the scope of this class.

As this is larger change to refactor it, I propose we handle it as separate refactoring effort to log properly with sink names. What do you think ?

Copy link
Owner

Choose a reason for hiding this comment

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

Couldn't a name field be added to MetricsSink or EndpointSink and referenced here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed by displaying the ClassName that is equivalent to name field. Can you please resolve this, if that's okay ?

@lightbend-cla-validator
Copy link

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

@hariprasad-k
Changes to support pushing lag metrics into InfluxDB
2841caf 
Merge 3e2f0bfaa3e646c27cb4158c0b547a2fcdd8fdea

Merge

Cosmetics

Syntax

Removing files

Refactored dependencies

Create db test

Removed unused import

Remove unused

Syntax

Cosmetics

Syntax

Removing files

Refactored dependencies

Create db test

Removed unused import

Changes to support pushing lag metrics into InfluxDB

Removed libraryDependencies from build.sbt
@hariprasad-k
Copy link
Contributor Author

Thanks for continuing this effort. I like the sink configuration re-factor. I'm not very familiar with influxdb so I'll need to rely on you to fill in the gaps.

Could you look into adding some tests?

@seglo Sure. We have also added Unit tests now to cover InfluxDB integration and followed up on all your other comments.

Can you please review again and let us know your thoughts ?

@hariprasad-k
Copy link
Contributor Author

@seglo Any thoughts/comments ?

seglo
seglo reviewed Nov 25, 2020
View reviewed changes
Copy link
Owner

@seglo seglo left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this forward. It's looking good. I left some comments.

src/main/scala/com/lightbend/kafkalagexporter/AppConfig.scala Show resolved Hide resolved
src/main/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSinkConfig.scala Outdated Show resolved Hide resolved
src/main/scala/com/lightbend/kafkalagexporter/MetricsReporter.scala Outdated
@@ -24,7 +24,7 @@ object MetricsReporter {
case (context, Stop(sender)) =>
Behaviors.stopped { () =>
metricsSink.stop()
context.log.info("Gracefully stopped Prometheus metrics endpoint HTTP server")
context.log.info("Gracefully stopped metrics sink")
Copy link
Owner

Choose a reason for hiding this comment

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

Couldn't a name field be added to MetricsSink or EndpointSink and referenced here?

src/test/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSinkTest.scala Outdated Show resolved Hide resolved
src/test/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSinkTest.scala Outdated Show resolved Hide resolved
@hariprasad-k
Addressed review comments to report MetricSink/EndPointSink name and …
0aba0e5 
…use ScalaTest eventually instead of a sleep.
@hariprasad-k
Copy link
Contributor Author

@seglo We have addressed your previous review comments. Can you please do once more pass and hopefully it's good to merge ?

seglo
seglo reviewed Dec 7, 2020
View reviewed changes
Copy link
Owner

@seglo seglo left a comment

Choose a reason for hiding this comment

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

Thanks. We're getting very close.

src/test/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSinkTest.scala Outdated Show resolved Hide resolved
src/test/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSinkTest.scala Outdated Show resolved Hide resolved
@hariprasad-k
Copy link
Contributor Author

Thanks. We're getting very close.

@seglo Glad to hear that we are getting very close. Addressed all your previous comments. Please review when you get a chance.

@seglo
Copy link
Owner

seglo commented Dec 17, 2020

Closing/opening ticket to try and trigger a build.

@seglo seglo closed this Dec 17, 2020
@seglo seglo reopened this Dec 17, 2020
@seglo seglo closed this Dec 17, 2020
@seglo seglo reopened this Dec 17, 2020
seglo
seglo approved these changes Dec 17, 2020
View reviewed changes
Copy link
Owner

@seglo seglo left a comment

Choose a reason for hiding this comment

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

LGTM

src/main/scala/com/lightbend/kafkalagexporter/AppConfig.scala Outdated Show resolved Hide resolved
src/main/scala/com/lightbend/kafkalagexporter/InfluxDBPusherSinkConfig.scala Outdated Show resolved Hide resolved
@seglo
Copy link
Owner

seglo commented Dec 17, 2020

One last thing. Can you update the README? Please add the same information as there is for graphite.

@hariprasad-k
Copy link
Contributor Author

One last thing. Can you update the README? Please add the same information as there is for graphite.

Good point. Updated the README. Please review if this looks good.

seglo
seglo reviewed Dec 17, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
@seglo seglo merged commit 998d58d into seglo:master Dec 17, 2020
@seglo
Copy link
Owner

seglo commented Dec 17, 2020

Looks good. Thanks!

@yehonatanz yehonatanz mentioned this pull request Feb 16, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@seglo seglo seglo 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.
3 participants
@hariprasad-k @lightbend-cla-validator @seglo