Skip to content

feat: adding consumer support for new traces/spans api #140

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

Merged
merged 4 commits into from
Jul 13, 2023

Conversation

kotharironak
Copy link
Contributor

Description

As part of supporting fetch_total as request in spans/traces API, this PR makes the corresponding changes at client side.

Ref PR: hypertrace/gateway-service#169

@@ -16,7 +16,7 @@ dependencies {
api("org.hypertrace.core.grpcutils:grpc-context-utils:0.12.1")
api("org.hypertrace.core.grpcutils:grpc-client-utils:0.12.1")
api("org.hypertrace.core.grpcutils:grpc-client-rx-utils:0.12.1")
api("org.hypertrace.gateway.service:gateway-service-api:0.2.25")
api("org.hypertrace.gateway.service:gateway-service-api:0.3.0-SNAPSHOT")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once I merged the gateway-service PR and cut the release, will update the appropriate version here.

return zip(
resultSetRequestBuilder.build(
context, HypertraceCoreAttributeScopeString.SPAN, arguments, spanAttributeExpressions),
logEventAttributeRequestBuilder.buildAttributeRequest(context, logAttributeExpressions),
(resultSetRequest, logEventAttributeRequest) ->
new DefaultSpanRequest(context, resultSetRequest, logEventAttributeRequest));
new DefaultSpanRequest(context, resultSetRequest, logEventAttributeRequest, false));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used via ExportSpans API, and ExportSpanResult doesn't extends ResultSet. It doesn't support total.

We can add it as a comment to explain why fetchTotal is set to false here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@skjindal93 skjindal93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good (apart from @satish-mittal's review comment)

@github-actions
Copy link

github-actions bot commented Jul 13, 2023

Test Results

110 tests  ±0   110 ✔️ ±0   35s ⏱️ ±0s
  38 suites ±0       0 💤 ±0 
  38 files   ±0       0 ±0 

Results for commit 8c60115. ± Comparison against base commit 984a7e2.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #140 (8c60115) into main (984a7e2) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #140   +/-   ##
=========================================
  Coverage     57.57%   57.57%           
  Complexity      264      264           
=========================================
  Files            91       91           
  Lines          1445     1445           
  Branches         42       42           
=========================================
  Hits            832      832           
  Misses          583      583           
  Partials         30       30           
Flag Coverage Δ
unit 57.57% <ø> (ø)

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

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kotharironak kotharironak requested a review from skjindal93 July 13, 2023 07:57
@kotharironak kotharironak merged commit 5a8d6d3 into main Jul 13, 2023
@kotharironak kotharironak deleted the update-spans-traces-client branch July 13, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants