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

PushTraceProto for batch uploading in a collector #249

Merged
merged 5 commits into from
Feb 7, 2020

Conversation

ibawt
Copy link
Contributor

@ibawt ibawt commented Feb 6, 2020

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
Copy link
Contributor Author

ibawt commented Feb 6, 2020

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
Copy link

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%) ⬇️
stackdriver.go 31.68% <0%> (-0.65%) ⬇️

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
Copy link
Contributor

rghetia commented Feb 6, 2020

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
Copy link
Contributor Author

ibawt commented Feb 6, 2020

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
Copy link
Contributor

rghetia commented Feb 6, 2020

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
Copy link
Contributor Author

ibawt commented Feb 7, 2020

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
Copy link
Contributor Author

ibawt commented Feb 7, 2020

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

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.

4 participants