-
Notifications
You must be signed in to change notification settings - Fork 8
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
#154 Log redis metrics to Eventbus #155
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #155 +/- ##
=============================================
+ Coverage 73.11% 77.75% +4.63%
+ Complexity 785 783 -2
=============================================
Files 65 63 -2
Lines 3407 3124 -283
Branches 354 306 -48
=============================================
- Hits 2491 2429 -62
+ Misses 682 475 -207
+ Partials 234 220 -14 ☔ View full report in Codecov by Sentry. |
public RedisMonitor(Vertx vertx, RedisProvider redisProvider, String name, int period, MetricsPublisher publisher) { | ||
this.vertx = vertx; | ||
this.redisProvider = redisProvider; | ||
this.period = period * 1000; |
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.
- this.period = period * 1000;
+ this.periodMs = periodSec * 1000;
Occurs several times.
(WARN: I did not verify if my assumed units are correct.)
public void stop() { | ||
if (timer != 0) { | ||
vertx.cancelTimer(timer); | ||
} |
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.
if (timer != 0) {
vertx.cancelTimer(timer);
+ timer = 0;
}
Just intuition. I don't know if this is needed or not.
private void collectMetrics(Buffer buffer) { | ||
Map<String, String> map = new HashMap<>(); | ||
|
||
Splitter.on(System.lineSeparator()).omitEmptyStrings() |
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.
Splitter.on(System.lineSeparator()).omitEmptyStrings()
^^^^^^^^^^^^^^^^^^^^^^
Are we sure we want an environmental condition in here? Usually this just helps that stuff "works on my machine" but then breaks on production because it fails to read the EXACT SAME input, but fails as it now uses another separator char.
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.
FYI the same code was added 2 years ago in Gateleen
} | ||
} catch (NumberFormatException e) { | ||
// ignore this field | ||
} |
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.
} catch (NumberFormatException e) {
- // ignore this field
+ log.debug("ignore field '{}' because '{}' doesnt look number-ish enough", key, valueStr);
}
@@ -379,6 +394,9 @@ private void unsupportedOperation(String operation, Message<JsonObject> event) { | |||
@Override | |||
public void stop() { | |||
unregisterConsumers(true); | |||
if(redisMonitor != null) { | |||
redisMonitor.stop(); | |||
} |
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.
if(redisMonitor != null) {
redisMonitor.stop();
+ redisMonitor = null;
}
Just an intuitive guess of mine. Maybe not needed.
<groupId>com.google.guava</groupId> | ||
<artifactId>guava</artifactId> | ||
<version>${guava.version}</version> | ||
</dependency> |
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.
FYI @ljucam @Kusig @linovifian
Just that you are aware what this means: This will make our houston and eagle transitively depend on guava, with no option to exclude the dependency because redisques would not work anymore then.
Maybe this could be a deal-breaker for PaISA. But as I'm not aware of how broad the impact will be, I'll mention you here.
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.
FYI the same code (RedisMonitor) already exists in gateleen. Since I don't wanted to add a dependency to gateleen, I duplicated the code in vertx-redisques
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.
We already have guava explicitly in Eagle, and in Houston we use the MonitoringHandler
from gateleen that transitievly uses guava.
We'd only have to check that this new explicit guava version doesn't break anything in these two services.
# Conflicts: # README.md # src/main/java/org/swisspush/redisques/RedisQues.java # src/test/java/org/swisspush/redisques/util/RedisquesConfigurationTest.java
No description provided.