Added optional "timestamp" value as part of metric sample by felipe-conde-benavides · Pull Request #337 · prometheus/client_java

felipe-conde-benavides · 2018-02-01T17:18:55Z

I'm using an exporter based on this code and it's working fine.

To be honest I'm not using equals() and hashCode() methods, so I haven't test it.

One more thing, about hashCode(): I'm not totally sure if code is totally agree with the current structure, so please check.

brian-brazil · 2018-02-01T17:26:03Z

This is breaking tests, and there should be unittests added for this.

It's be better to take a unboxed value.

pjfanning · 2018-02-02T10:10:37Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

          return false;
        }
        Sample other = (Sample) obj;
+        boolean timestampEquals = true;


This is the Java 7 java.utils.Objects.equals. This project maintains Java 6 compatibility but it might be tidier to paste in this method and use it compare all the member objects using this method.

static boolean equals(Object a, Object b) { return (a == b) || (a != null && a.equals(b)); }

I don't totally understand what you say:

boolean timestampEquals = true; if ((timestamp != null && other.timestamp == null) || (timestamp == null && other.timestamp != null)) { timestampEquals = false; } if (timestamp != null && other.timestamp != null) { timestampEquals = other.timestamp.equals(timestamp); }

The equals() used here is the same used in other parts of previously existing code, such as:

return other.name.equals(name) && other.labelNames.equals(labelNames) && other.labelValues.equals(labelValues) && other.value == value;

My suggestion is to based on readability - but it's just a suggestion.

pjfanning · 2018-02-02T10:15:52Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

        hash = 37 * hash + labelValues.hashCode();
        long d = Double.doubleToLongBits(value);
        hash = 37 * hash + (int)(d ^ (d >>> 32));
+        hash = 37 * hash + timestamp.hashCode();


this needs to be wrapped in an if (timestamp != null) check

Thank you, I have changed it, but still not pushed.

pjfanning · 2018-02-02T10:18:13Z

simpleclient_common/src/main/java/io/prometheus/client/exporter/common/TextFormat.java

        writer.write(' ');
        writer.write(Collector.doubleToGoString(sample.value));
+        if (sample.timestamp != null){
+        	writer.write(' ');


could you remove the tab and use spaces for indents?

Sorry, I didn't realize about it, I have changed it, but still not pushed.

pjfanning · 2018-02-02T12:31:28Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

        long d = Double.doubleToLongBits(value);
        hash = 37 * hash + (int)(d ^ (d >>> 32));
+        if (timestamp != null)
+        	hash = 37 * hash + timestamp.hashCode();


could you remove the tab here?

Sorry, now my Eclipse IDE is configured for use "spaces" (2) instead of "tabs".

pjfanning · 2018-02-02T12:31:55Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

        return other.name.equals(name) && other.labelNames.equals(labelNames)
-          && other.labelValues.equals(labelValues) && other.value == value;
+          && other.labelValues.equals(labelValues) && other.value == value
+		  && timestampEquals;


also appears to be a tab here

Sorry, now my Eclipse IDE is configured for use "spaces" (2) instead of "tabs".

pjfanning · 2018-02-02T12:59:22Z

@efelcon would you have time to add the unit tests that Brian asked for?

felipe-conde-benavides · 2018-02-02T13:08:04Z

I have the unit tests creation pending, now I have no time to complete it, but I expect to complete before weekend.

brian-brazil · 2018-02-02T14:09:53Z

I'm at FOSDEM and unlikely to give this a proper look until Tuesday.

…

On 2 Feb 2018 14:08, "Felipe Conde" ***@***.***> wrote: I have the unit tests creation pending, now I have no time to complete it, but I expect to complete before weekend. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#337 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGyTdgOnNb2LVkYcqSiaizXLW5DplvZHks5tQwi1gaJpZM4R1-ju> .

pjfanning · 2018-02-05T11:31:52Z

simpleclient/src/test/java/io/prometheus/client/CounterTest.java


+  @Test
+  public void testCollectWithTimestamp() {
+  	Long now = System.currentTimeMillis();


another tab

brian-brazil · 2018-02-07T11:59:45Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

      public final List<String> labelNames;
      public final List<String> labelValues;  // Must have same length as labelNames.
      public final double value;
+      public final Long timestamp;


timestampMs would be clearer.

Can you also add a note that this field is subject to change?

I'm agree, this has been changed.

brian-brazil · 2018-02-07T12:02:47Z

simpleclient/src/main/java/io/prometheus/client/Collector.java

        }
        Sample other = (Sample) obj;
+
+        boolean timestampEquals = true;


This is a bit long, I think you can simplify a bit.

Done. Now has been reduced to one line.

brian-brazil · 2018-02-07T12:03:27Z

simpleclient/src/main/java/io/prometheus/client/Counter.java

+    /**
+     * Set the optional value for time stamp in epoch format.
+     */
+    public void setTimestamp(Long timestamp) {


Direct instrumentation has no need for timestamps, these need to be removed.

If I remove this, the Unit Tests for Counter called testCollectWithTimestamp must be removed, because there will be no way for setting the timestamp property (also happen the same for Gauge)). For me there is no problem so, are you agree to remove also the both Unit Tests called testCollectWithTimestamp?

Timestamps only work via custom collectors, so tests need to implement one of those.

Ok, now I think I understand what you mean. But I'm afraid I have commited several changes. Tomorrow I will work on this.

brian-brazil · 2018-02-07T12:03:52Z

simpleclient_common/src/main/java/io/prometheus/client/exporter/common/TextFormat.java

        writer.write(Collector.doubleToGoString(sample.value));
+        if (sample.timestamp != null){
+          writer.write(' ');
+          writer.write(sample.timestamp.toString());


This needs unittests

Refactoring of some code in equals method.

…to "value" functions, such as get()/set()

…ture used by "value". This is needed for Unit Tests code.

…he new functions added (covering both cases: "labels" and "nolabels").

felipe-conde-benavides · 2018-02-12T19:50:15Z

I have added a Unit Test for TextFormat java class (I'm not totally sure if it's all what you want).

Please confirm me if you want me to remove the methods and Unit Tests added in Counter, CounteTest, Gauge and GaugeTest classes ("timestamp" field related).

brian-brazil

Yes, those should be removed.

brian-brazil · 2018-02-13T11:40:34Z

simpleclient_common/src/test/java/io/prometheus/client/exporter/common/TextFormatTest.java

+  }
+
+  @Test
+  public void testMetricOutputWithTimetamp() throws IOException {


felipe-conde-benavides · 2018-02-13T20:24:50Z

@brian-brazil I have removed all code you request to remove, so I think that all is ready for your check.

brian-brazil · 2018-02-14T09:30:58Z

simpleclient_common/src/test/java/io/prometheus/client/exporter/common/TextFormatTest.java

+    TextFormat.write004(writer, registry.metricFamilySamples());
+    assertEquals("# HELP nolabels help\n"
+                 + "# TYPE nolabels untyped\n"
+                 + "nolabels 1.0 "+ now +"\n", writer.toString());


I'd prefer this was a literal string rather than dynamic, as the output here depends on Java's implicit conversion to string.

brian-brazil · 2018-02-14T10:11:26Z

Thanks!

Added optional "timestamp" value as part of metric sample

eb2d8d1

Method "equals" was wrong

4a0db48

pjfanning reviewed Feb 2, 2018

View reviewed changes

Felipe Conde added 2 commits February 2, 2018 13:26

changed "tabs" for "spaces"

3510513

Added extra check for "null" values

d0ffe6a

pjfanning reviewed Feb 2, 2018

View reviewed changes

changed "tabs" for "spaces"

2acdc29

Felipe Conde added 3 commits February 5, 2018 12:27

Added "timestamp" field into required classes

b7e20df

Fixed code format.

70fd6db

Added unit tests

7ff6b06

pjfanning reviewed Feb 5, 2018

View reviewed changes

Changed "tabs" for "spaces"

873e361

brian-brazil reviewed Feb 7, 2018

View reviewed changes

Felipe Conde added 7 commits February 7, 2018 20:32

Renamed timestamp property to timestampMs (Added also a note).

908031e

Refactoring of some code in equals method.

Minor change: Just moved function setTimeStamp() to be located close …

3dc7855

…to "value" functions, such as get()/set()

Added also getTimestamp()/setTimestamp() to be aligned with the struc…

88ee313

…ture used by "value". This is needed for Unit Tests code.

Fixed, property name was changed on "simpleclient" project but not here.

9bf1159

Fixed and improved Unit Tests with new property name and also using t…

ee11fb7

…he new functions added (covering both cases: "labels" and "nolabels").

Added 2 new Unit Tests using timestamps.

3af44d7

Unit test added for metric with timestamp

5d6af43

brian-brazil reviewed Feb 13, 2018

View reviewed changes

Removed "timestamp" methods and Unit Tests for "Counter" and "Gauge"

143bc07

Typo fixed

c463ae6

brian-brazil reviewed Feb 14, 2018

View reviewed changes

Changed timestamp to fixed value instead of dynamic value

aae6c18

brian-brazil merged commit 5b0a275 into prometheus:master Feb 14, 2018

felipe-conde-benavides deleted the feature-timestamp branch February 14, 2018 10:27

This was referenced May 29, 2018

exporter/stats/prometheus: java.lang.NoSuchFieldError: timestampMs census-instrumentation/opencensus-java#1215

Closed

Got lots of warning logs (java.lang.NoSuchFieldError: timestampMs) when timestamp field is unset #384

Closed

Conversation

felipe-conde-benavides commented Feb 1, 2018

Uh oh!

brian-brazil commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjfanning commented Feb 2, 2018

Uh oh!

felipe-conde-benavides commented Feb 2, 2018

Uh oh!

brian-brazil commented Feb 2, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipe-conde-benavides commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-brazil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipe-conde-benavides commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-brazil commented Feb 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

felipe-conde-benavides commented Feb 12, 2018 •

edited

Loading

felipe-conde-benavides commented Feb 13, 2018 •

edited

Loading