census-ecosystem / opencensus-go-exporter-stackdriver

OpenCensus Go exporter for Stackdriver Monitoring and Trace
Apache License 2.0
67 stars 79 forks source link

PushTraceProto for batch uploading in a collector #249

Closed ibawt closed 4 years ago

ibawt commented 4 years ago

What

In order to support opentelemetry collector batching queueing etc better I added this to the exporter.

I'm not sure what to do with Node and Resource really. If you have any guidance I can write the code. OTOH this is what I'm using currently and losing the resource anyways so it doesn't matter too much to me.

I'm not sure if you'll like the dependency on OTel vs OC though, in practice I'm not sure if it matters much. It does allow me to convert straight into the output array vs yet another intermediate array to store the spandata's.

ibawt commented 4 years ago

fwiw we (Shopify) are getting loads of buffer fulls from the way the current exporter is done. I proposed adding more options to the exporter code, but was directed here instead.

codecov-io commented 4 years ago

Codecov Report

Merging #249 into master will decrease coverage by 1.28%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   73.22%   71.93%   -1.29%     
==========================================
  Files          14       14              
  Lines        1621     1650      +29     
==========================================
  Hits         1187     1187              
- Misses        358      387      +29     
  Partials       76       76
Impacted Files Coverage Δ
trace.go 46.15% <0%> (-13.85%) :arrow_down:
stackdriver.go 31.68% <0%> (-0.65%) :arrow_down:

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 a428e35...d3abd69. Read the comment docs.

rghetia commented 4 years ago

I'm not sure what to do with Node and Resource really. If you have any guidance I can write the code. OTOH this is what I'm using currently and losing the resource anyways so it doesn't matter too much to me.

You can ignore the node but use the resource if not nil.

I'm not sure if you'll like the dependency on OTel vs OC though, in practice I'm not sure if it matters much. It does allow me to convert straight into the output array vs yet another intermediate array to store the spandata's.

I see that it is pulling lot of dependencies due to OTel. It may become an issue for those who are using the exporter directly (without otel collector).

ibawt commented 4 years ago

I can do the ocspan -> spandata translation in the collector /exporter. It won't be technically a tracepb.Span then though ( not sure it matters, maybe on the function name). The only disadvantage is the creation of one extra array vs the inline conversion in here. It probably doesn't matter in practice. If I see it on a production profile I will revisit.

rghetia commented 4 years ago

I can do the ocspan -> spandata translation in the collector /exporter. It won't be technically a tracepb.Span then though ( not sure it matters, maybe on the function name). The only disadvantage is the creation of one extra array vs the inline conversion in here. It probably doesn't matter in practice. If I see it on a production profile I will revisit.

Perhaps change the fn name to PushSpanData.

ibawt commented 4 years ago

I changed it , called it "PushTraceSpans", can change it to whatever. I also did the resource thing, like how the metrics code did it.

I kept the interface of returning (int, error) as thats' similar to the otel col api. Not sure if we should keep it really as it's really only useful for returning span translation errors and that happens on the other end now.

Let me know if you want some tests, but they aren't very useful here imo.

ibawt commented 4 years ago

also I ran go mod tidy after removing otel, and it updated a bunch more things, seems legit though.