Changes for duration from milli to micro by TriptiTripathi1234 · Pull Request #32 · razorpay/hypertrace-ingester

TriptiTripathi1234 · 2022-08-24T06:05:45Z

Duration is in milli second currently. Changes are done to change in to micro seconds.

suddendust

Please add the appropriate test cases.

Sunn-y-Arora · 2022-08-24T09:44:27Z

 pinot.timeUnit = MILLISECONDS
 # todo: Add Attributes and Metrics after adding map type value
-pinot.dimensionColumns = [tenant_id, span_kind, error_count, exception_count, duration_millis, end_time_millis, api_name, service_name, span_id, trace_id, protocol_name, status_code, service_id, api_id, num_calls, api_discovery_state, space_ids]
+pinot.dimensionColumns = [tenant_id, span_kind, error_count, exception_count, duration_millis, duration_micros, end_time_millis, api_name, service_name, span_id, trace_id, protocol_name, status_code, service_id, api_id, num_calls, api_discovery_state, space_ids]


why do we need this duration in micro across views, I think as per my understanding, only the span event view needed that (and the backend entity view).

@Sunn-y-Arora I think it's better to have this consistently across all views.

not required right? raw-service (for eg) view gives service ka overall rsp time, it will never be in micro, so unnecessary change

True but why have two different units in two different views? We'll anyway get rid of duration_millis across all tables, if extra storage and indexing cost is a concern. This also makes code the query side simpler, as it doesn't have to differentiate b/w the two columns IMO.

As pointed out by Prashant, added it in micro across views so that all the views have consistency.

codecov-commenter · 2022-08-29T10:35:27Z

Codecov Report

❗ No coverage uploaded for pull request base (rzp_main@158eb5a). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 68065ea differs from pull request most recent head 553a85f. Consider uploading reports for the commit 553a85f to get more accurate results

@@             Coverage Diff             @@
##             rzp_main      #32   +/-   ##
===========================================
  Coverage            ?   79.45%           
  Complexity          ?     1311           
===========================================
  Files               ?      118           
  Lines               ?     5271           
  Branches            ?      481           
===========================================
  Hits                ?     4188           
  Misses              ?      865           
  Partials            ?      218

Flag	Coverage Δ
unit	`79.45% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

TriptiTripathi1234 · 2022-09-06T06:54:10Z

Please add the appropriate test cases.

added @suddendust

suddendust · 2022-09-06T07:00:14Z

    long duration_millis = 0;

+    // duration micro
+    double duration_micros = 0;


This can be long?

As per our discussion, we went with double to store all the significant digits(might need in future). Making it LONG will defeat this purpose.

Long's upper limit is 9223372036854775807, which is equal to 292471.20867753599305 years. Do you think a span's duration will exceed this value?

we are talking about significant digits, not the range

let's say we have .123 ms that would be 0ms(in LONG), 123 micro seconds(LONG).
Now if we have .1234ms that would be 123 micro seconds(if LONG), 123.4 micro seconds(if DOUBLE). Isnt it?

Aren't we solving for microsecond granularity? It means the smallest duration we can report is 1µ. So 123.4 µs should be rounded off to 123 µs. This will keep things on the query layer simples as you wouldn't have to deal with converting data types.

As per discussion with team and tech specs I have added datatype as DOUBLE

suddendust · 2022-09-06T07:02:34Z

    long duration_millis;

+    // duration micro
+    double duration_micros = 0;


suddendust · 2022-09-06T07:02:59Z

      builder.setStartTimeMillis(event.getStartTimeMillis());
      builder.setEndTimeMillis(event.getEndTimeMillis());
      builder.setDurationMillis(event.getEndTimeMillis() - event.getStartTimeMillis());
+      builder.setDurationMicros(event.getMetrics().getMetricMap().get("Duration").getValue());


Move this to constants, or reuse if this is already defined.

suddendust · 2022-09-06T07:03:06Z

        builder.setStartTimeMillis(event.getStartTimeMillis());
        builder.setEndTimeMillis(event.getEndTimeMillis());
        builder.setDurationMillis(event.getEndTimeMillis() - event.getStartTimeMillis());
+        builder.setDurationMicros(event.getMetrics().getMetricMap().get("Duration").getValue());


suddendust · 2022-09-06T07:04:34Z

    MetricValue durationMetric =
        fastNewBuilder(MetricValue.Builder.class)
-            .setValue((double) (endTimeMillis - startTimeMillis))
+            .setValue(


Also a test case for this?

suddendust

LGTM, requesting minor changes.

github-actions · 2022-09-27T05:11:47Z

Unit Test Results

  76 files ±0   76 suites ±0 1m 9s ⏱️ -1s
398 tests ±0 396 ✔️ ±0 2 💤 ±0 0 ❌ ±0

Results for commit 68065ea. ± Comparison against base commit 92601e7.

Changes for duration from milli to micro

c5dcd32

TriptiTripathi1234 changed the title ~~Changes for duration from milli to micro~~ WIP: Changes for duration from milli to micro Aug 24, 2022

TriptiTripathi1234 requested review from Sunn-y-Arora and suddendust August 24, 2022 09:22

suddendust suggested changes Aug 24, 2022

View reviewed changes

Sunn-y-Arora reviewed Aug 24, 2022

View reviewed changes

test cases fix

24b7844