feat: Api for log event by findingrish · Pull Request #87 · hypertrace/gateway-service

findingrish · 2021-04-15T08:29:01Z

Adds LogEvents api

codecov · 2021-04-15T08:30:28Z

Codecov Report

Merging #87 (3c049f4) into main (27c0e47) will decrease coverage by 0.06%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##               main      #87      +/-   ##
============================================
- Coverage     77.08%   77.02%   -0.07%     
- Complexity     1082     1094      +12     
============================================
  Files           105      106       +1     
  Lines          4884     4980      +96     
  Branches        416      424       +8     
============================================
+ Hits           3765     3836      +71     
- Misses          936      955      +19     
- Partials        183      189       +6

Flag	Coverage Δ	Complexity Δ
unit	`77.02% <75.00%> (-0.07%)`	`1094.00 <12.00> (+12.00)`	⬇️

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

Impacted Files	Coverage Δ	Complexity Δ
...hypertrace/gateway/service/GatewayServiceImpl.java	`18.45% <5.55%> (-1.55%)`	`3.00 <0.00> (ø)`
...ace/gateway/service/logevent/LogEventsService.java	`89.61% <89.61%> (ø)`	`11.00 <11.00> (?)`
...way/service/common/util/AttributeMetadataUtil.java	`83.33% <100.00%> (+0.47%)`	`8.00 <1.00> (+1.00)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27c0e47...3c049f4. Read the comment docs.

aaron-steinfeld · 2021-04-15T19:06:17Z

+
+message LogEventResponse {
+  repeated LogEvent logEvents = 1;
+  int32 total = 2;


My understanding is this needs to be fetched separately, so we've kind of retrofitted an "includeTotal" flag on other APIs. Since this is a new one, it may be worth looking at those and if it makes sense, splitting fetching total into its own API.

Why does total needs to be fetched separately? and if that is added, should the existing api also return total?

It was a perf optimization. In many contexts (for example, showing the log events in the sequence diagram), we don't need the total - but under the hood, the gateway service had to make a separate query (which I believe was sequential, although that could be fixed) to fetch it and it slowed down the queries that didn't need it. By keeping it separate, we can fetch it in parallel if needed and leave it be if it's not requested. @skjindal93 may have more context.

Why does total needs to be fetched separately? and if that is added, should the existing api also return total?

I think there are 2 questions here

Why does total needs to be fetched separately?
If the request has support for limit and offset, which usually gets pushed down to the data store, you cannot count number of log events in the response and call it as total (since the count of log events would just be bounded by the limit)
A separate request needs to be made to get the total value, by just passing the filters sent in the request (And, that's why I personally prefer to have a separate total request API entirely)

What if you don't need total in the response?
That's the perf optimization which @aaron-steinfeld was mentioning about. Since, in order to fetch total, you need to make a separate request, but if the consumer does not need it, we should skip fetching the total entirely (by either having a boolean flag like this https://github.com/hypertrace/gateway-service/blob/main/gateway-service-api/src/main/proto/org/hypertrace/gateway/service/v1/entities.proto#L50 or a separate total request API)

Initially, since I didn't add support for limit and order, calculating total was trivial. But, yeah with limit and order it needs another query.
For this change, I will remove total from the api.

aaron-steinfeld · 2021-04-20T19:33:55Z

+
+message LogEventsRequest {
+  // todo change this to nano
+  sfixed64 start_time_nanos = 1;


hmmm - to keep this in nanos or millis? My gut is millis so this becomes more of an implementation detail (since the unit on the attribute itself will be handled generically), but I'm on the fence.

Ok, I can do that. Currently I am just converting timestamp to nanos in gql request builder.

aaron-steinfeld · 2021-04-20T19:38:20Z

  public static Filter addTimeAndSpaceFiltersAndConvertToQueryFilter(
-      long startTimeMillis,
-      long endTimeMillis,
+      long startTime,


I don't believe this util cares about units, but just to be aware, others do (anything using datetimeconvert). We'll probably want to lookup the timestamp unit when we get to that point.

aaron-steinfeld · 2021-04-21T19:06:04Z

  repeated double double_array = 13;
  repeated bool boolean_array = 14;
  map<string, string> string_map = 15;
+  string timestamp_unit = 16;


Why do we need this in the value? The requester has access to the metadata anyway so should have this info (which saves us from repeating the unit in every response value - we already do this for all other units)

Yeah that can also be done. But I will then need a different converter for Timestamp in gql, here hypertrace/hypertrace-core-graphql#56 (comment) since the convert method only has access to the Value object.

we already do this for all other units)

where do we do this?

My response from offline:

So GQL will need changes to support timestamps as instants in attributes - right now it only works that way for top level APIs. The value converter needs to be changed to a biconverter I believe to take in the attribute metadata. Or we may need both the Converter and Biconverter flavors if there’s some places we call it where we don’t have that metadata at hand (probably just implement both methods and have an internal implementation, for example). The 400 error is due to the serialization side. Attributes right now use an UnknownScalar type, which needs to add support for instants - right now it treats them as plain objects.

As to where we do this, I actually confused units and types. The units being joined in happens in the UI today - the types being joined in we do in GQL explorer results.

skjindal93 · 2021-04-19T13:51:47Z

+}
+
+message LogEventResponse {
+  repeated LogEvent log_events = 1;


no total? since, we have offset and limit now, I believe it will have pagination. That will need a total in the response

So, that would need a parallel query I guess

Yes. That works too. Definitely, we can defer it as a separate PR

👍 that's what I was thinking - separate api entirely since we have the luxury of doing it here.

Co-authored-by: Aaron Steinfeld <45047841+aaron-steinfeld@users.noreply.github.com>

…log-api

github-actions · 2021-04-22T17:00:07Z

Unit Test Results

  55 files +1   55 suites +1 6s ⏱️ -1s
315 tests +1 315 ✔️ +1 0 💤 ±0 0 ❌ ±0

Results for commit 9e7f586. ± Comparison against base commit 27c0e47.

Api for log event

e9ab15c

This comment has been minimized.

Sign in to view

rish691 added 2 commits April 15, 2021 14:02

Merge remote-tracking branch 'origin/main' into log-api

273f82b

Comment out code in LogEventService for now

b9766b2

findingrish requested review from aaron-steinfeld and kotharironak April 15, 2021 18:41

findingrish marked this pull request as ready for review April 15, 2021 18:41

findingrish requested review from avinashkolluru and tim-mwangi as code owners April 15, 2021 18:41

This comment has been minimized.

Sign in to view

spotless

8fe9faf

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Apr 15, 2021

View reviewed changes

Remove total

171e2a4

This comment has been minimized.

Sign in to view

spotless

3dc464a

This comment has been minimized.

Sign in to view

skjindal93 reviewed Apr 16, 2021

View reviewed changes

Comment thread gateway-service-api/src/main/proto/org/hypertrace/gateway/service/v1/log_event.proto Outdated

tim-mwangi reviewed Apr 16, 2021

View reviewed changes

Comment thread gateway-service-impl/src/main/java/org/hypertrace/gateway/service/logevent/LogEventService.java Outdated

rish691 added 2 commits April 19, 2021 18:54

Add impl for LogEventService

ce5bfeb

Update log event api

35cf482

This comment has been minimized.

Sign in to view

.

e1a477a

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Apr 19, 2021

View reviewed changes

Add test

2882e23

This comment has been minimized.

Sign in to view

Update timestamp config to use scope instead of scope_string

1a566d1

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Apr 20, 2021

View reviewed changes

Add timeunit in Value proto

573e166

findingrish requested a review from a team April 21, 2021 16:52

rish691 added 2 commits April 21, 2021 23:57

Handle time conversion in LogEvents request/response

5e264da

Merge remote-tracking branch 'origin/main' into log-api

7feeeea

This comment has been minimized.

Sign in to view

.

4c98689

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Apr 21, 2021

View reviewed changes

skjindal93 reviewed Apr 22, 2021

View reviewed changes

Revert change in Value proto

9981f78

This comment has been minimized.

Sign in to view

aaron-steinfeld previously approved these changes Apr 22, 2021

View reviewed changes

Comment thread ...way-service-impl/src/main/java/org/hypertrace/gateway/service/logevent/LogEventsService.java Outdated

Comment thread ...way-service-impl/src/main/java/org/hypertrace/gateway/service/logevent/LogEventsService.java Outdated

.

057fce7

Co-authored-by: Aaron Steinfeld <45047841+aaron-steinfeld@users.noreply.github.com>

findingrish dismissed aaron-steinfeld’s stale review via 057fce7 April 22, 2021 15:46

This comment has been minimized.

Sign in to view

rish691 added 2 commits April 22, 2021 21:33

.

ee97035

Merge branch 'log-api' of github.com:hypertrace/gateway-service into …

3c049f4

…log-api

This comment has been minimized.

Sign in to view

aaron-steinfeld approved these changes Apr 22, 2021

View reviewed changes

findingrish merged commit 9e7f586 into main Apr 22, 2021

findingrish deleted the log-api branch April 22, 2021 16:58

Conversation

findingrish commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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!

skjindal93 Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findingrish Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

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!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

findingrish Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

skjindal93 Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions Bot commented Apr 22, 2021

Unit Test Results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

findingrish commented Apr 15, 2021 •

edited

Loading

codecov Bot commented Apr 15, 2021 •

edited

Loading

skjindal93 Apr 16, 2021 •

edited

Loading

findingrish Apr 16, 2021 •

edited

Loading

findingrish Apr 21, 2021 •

edited

Loading

skjindal93 Apr 22, 2021 •

edited

Loading