Add support for start and end times in count and histogram aggregate actions #4614
Conversation
kkondaka
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
asifsmohammed,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
| @@ -134,6 +135,23 @@ public static long getTimeNanos(final Instant time) { | |||
| return currentTimeNanos; | |||
| } | |||
|
|
|||
| public static Instant convertObjectToInstant(Object timeObject) { | |||
There was a problem hiding this comment.
This should have a unit test specifically for this.
Also, you can add this as a InstantHelper to the actions package and make it package-protected. That way it doesn't expand much.
There was a problem hiding this comment.
I kept it here because this may become useful at aggregate processor level at a later time.
| int idx = Arrays.binarySearch(this.buckets, doubleValue); | ||
| if (idx < 0) { | ||
| idx = -idx-2; | ||
| } | ||
| Instant eventTime = Instant.now(); | ||
| Instant eventStartTime = Instant.now(); |
There was a problem hiding this comment.
Let's set these to the exact same value instead of calling Instant.now() multiple times.
Instant eventStartTime = eventTime;
Instant eventEndTime = eventTime;
| @@ -81,15 +79,33 @@ public Exemplar createExemplar(final Event event) { | |||
| @Override | |||
| public AggregateActionResponse handleEvent(final Event event, final AggregateActionInput aggregateActionInput) { | |||
| final GroupState groupState = aggregateActionInput.getGroupState(); | |||
| Instant eventStartTime = Instant.now(); | |||
| groupState.put(endTimeKey, Instant.now()); | ||
| Instant groupStartTime = (Instant)groupState.get(startTimeKey); | ||
| Instant groupEndTime = (Instant)groupState.get(endTimeKey); | ||
| if (eventStartTime.isBefore(groupStartTime)) |
There was a problem hiding this comment.
Are these conditions being tested? Both true and false?
There was a problem hiding this comment.
Yes because the test cases assign the times with random delta.
Instant sTime = (i == 0) ? testTime : testTime.plusSeconds(random.nextInt(5));
Instant eTime = (i == testCount-1) ? testTime.plusSeconds(100) : testTime.plusSeconds (50+random.nextInt(45));
Signed-off-by: Krishna Kondaka <[email protected]>
kkondaka
force-pushed
the
aggregate-times
branch
from
093e9a8 to
691e7e5
Compare
| if (eventStartTime.isBefore(groupStartTime)) | ||
| groupState.put(startTimeKey, eventStartTime); | ||
| if (eventEndTime.isAfter(groupEndTime)) | ||
| groupState.put(endTimeKey, eventEndTime); |
There was a problem hiding this comment.
This seems like generic aggregation functionality where we just track the range of times the events have come from for that aggregated event. Is there a way to extend this to all aggregate actions with some common code? Or is it only applicable to count and histogram actions?
| @ParameterizedTest | ||
| @ValueSource(ints = {10, 20, 50, 100}) | ||
| void testHistogramAggregateOTelFormatWithStartAndEndTimesInTheEvent(int testCount) throws NoSuchFieldException, IllegalAccessException { | ||
| HistogramAggregateActionConfig mockConfig = mock(HistogramAggregateActionConfig.class); |
There was a problem hiding this comment.
This test is hard to read. Might be worth creating some private functions to make it more readable.
Description
Add support for start and end times in count and histogram aggregate actions.
Count and Histogram aggregate actions include start and end times based on the real time of aggregation. Adding support to include start and end times based on the times in the event.
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.