Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

update PushTraceSpans to return dropped spans instead of good spans. #251

Merged
merged 2 commits into from
Feb 7, 2020

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Feb 7, 2020

fixes #250

@rghetia
Copy link
Contributor Author

rghetia commented Feb 7, 2020

@ibawt PTAL.

@rghetia rghetia requested a review from dinooliva February 7, 2020 19:13
Copy link
Contributor

@ibawt ibawt left a comment

Choose a reason for hiding this comment

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

lgtm

@rghetia rghetia requested a review from songy23 February 7, 2020 19:22
@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #251 into master will decrease coverage by 0.13%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
- Coverage   72.24%   72.11%   -0.14%     
==========================================
  Files          14       14              
  Lines        1643     1646       +3     
==========================================
  Hits         1187     1187              
- Misses        380      383       +3     
  Partials       76       76
Impacted Files Coverage Δ
trace.go 47.78% <0%> (-1.31%) ⬇️
stackdriver.go 31.68% <0%> (ø) ⬆️

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 ab68e2a...0ce3886. Read the comment docs.

trace.go Outdated
@@ -148,7 +148,8 @@ func (e *traceExporter) pushTraceSpans(ctx context.Context, node *commonpb.Node,
ctx, cancel := newContextWithTimeout(ctx, e.o.Timeout)
defer cancel()

return e.client.BatchWriteSpans(ctx, &req)
err := e.client.BatchWriteSpans(ctx, &req)
return 0, err
Copy link

@dinooliva dinooliva Feb 7, 2020

Choose a reason for hiding this comment

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

There will never be any dropped spans for stackdriver, is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if err is not nil, we should return len(protoSpans) 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.

@dinooliva it will be all or none.

@songy23 fixed it.

@rghetia rghetia merged commit ddbc64a into census-ecosystem:master Feb 7, 2020
@rghetia rghetia deleted the fix_push_proto branch February 7, 2020 23:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PushTraceSpans should return dropped spans instead of good spans
6 participants