-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Integrate with Gateway log api #56
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
Conversation
...e-utils/src/main/java/org/hypertrace/core/graphql/utils/gateway/UnwrappedValueConverter.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...src/main/java/org/hypertrace/core/graphql/common/request/DefaultAttributeRequestBuilder.java
Outdated
Show resolved
Hide resolved
...e-utils/src/main/java/org/hypertrace/core/graphql/utils/gateway/UnwrappedValueConverter.java
Outdated
Show resolved
Hide resolved
...e-utils/src/main/java/org/hypertrace/core/graphql/utils/gateway/UnwrappedValueConverter.java
Outdated
Show resolved
Hide resolved
...e-utils/src/main/java/org/hypertrace/core/graphql/utils/gateway/UnwrappedValueConverter.java
Outdated
Show resolved
Hide resolved
...chema/src/main/java/org/hypertrace/core/graphql/log/event/dao/GatewayServiceLogEventDao.java
Outdated
Show resolved
Hide resolved
...java/org/hypertrace/core/graphql/log/event/dao/GatewayServiceLogEventsResponseConverter.java
Outdated
Show resolved
Hide resolved
...java/org/hypertrace/core/graphql/log/event/dao/GatewayServiceLogEventsResponseConverter.java
Show resolved
Hide resolved
...vent-schema/src/main/java/org/hypertrace/core/graphql/log/event/fetcher/LogEventFetcher.java
Outdated
Show resolved
Hide resolved
...vent-schema/src/main/java/org/hypertrace/core/graphql/log/event/fetcher/LogEventFetcher.java
Outdated
Show resolved
Hide resolved
|
Build would pass once the gateway pr is merged and its version is updated. |
...ice-utils/src/main/java/org/hypertrace/core/graphql/utils/gateway/WrappedValueConverter.java
Outdated
Show resolved
Hide resolved
| new Coercing<>() { | ||
| @Override | ||
| public Object serialize(Object fetcherResult) throws CoercingSerializeException { | ||
| if (fetcherResult instanceof Instant) { |
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'll need to be handle the other direction, too. Both from the transport string to instant, and from instant to the gateway format. I'm ok deferring that if you want to leave a TODO since the only place where this would come up is passing in values to filters, but we generally filter timestamps via the first class time range.
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.
yeah, that is why I skipped. I will add a todo at the moment.
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.
, and from instant to the gateway format.
What do you mean by this? instant to gateway format conversion isn't done 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.
LiteralConstantConverter - the conversion from a java primitive/object into the gateway argument format. This spot does the conversion from the serialized format from http into the java primitive/object.
...ice-utils/src/main/java/org/hypertrace/core/graphql/utils/gateway/AttributeMapConverter.java
Outdated
Show resolved
Hide resolved
...eway-service-utils/src/main/java/org/hypertrace/core/graphql/utils/gateway/ValueWrapper.java
Outdated
Show resolved
Hide resolved
...java/org/hypertrace/core/graphql/log/event/dao/GatewayServiceLogEventsResponseConverter.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/hypertrace/core/graphql/log/event/request/DefaultLogEventRequestBuilder.java
Show resolved
Hide resolved
...c/main/java/org/hypertrace/core/graphql/log/event/request/DefaultLogEventRequestBuilder.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #56 +/- ##
============================================
- Coverage 61.83% 61.48% -0.35%
- Complexity 230 233 +3
============================================
Files 77 77
Lines 1158 1171 +13
Branches 31 33 +2
============================================
+ Hits 716 720 +4
- Misses 417 423 +6
- Partials 25 28 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
aaron-steinfeld
left a comment
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.
lgtm - could you add a couple strategic tests (maybe the timestamp conversion, request and response builders)?
This comment has been minimized.
This comment has been minimized.
Added, let me know if this is fine. |
hypertrace/hypertrace#224
Request/Response: