Skip to content
This repository has been archived by the owner on Nov 7, 2022. It is now read-only.

Jaeger grpc exporter #433

Merged
merged 63 commits into from
Mar 14, 2019

Conversation

annanay25
Copy link
Contributor

Resolves #279

Added oc-proto to jaeger-proto translation, and jaeger-grpc sender.

@annanay25 annanay25 requested review from pjanotti and a team as code owners February 24, 2019 10:48
@flands
Copy link
Contributor

flands commented Feb 24, 2019

Nice!

@flands flands added this to the 0.2.0 milestone Feb 24, 2019
Copy link

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

I still not finished but want to provide early feedback

cmd/occollector/app/sender/jaeger_proto_grpc_sender.go Outdated Show resolved Hide resolved
cmd/occollector/app/sender/jaeger_proto_grpc_sender.go Outdated Show resolved Hide resolved
cmd/occollector/app/sender/jaeger_proto_grpc_sender.go Outdated Show resolved Hide resolved
translator/trace/jaeger/protospan_to_jaegerproto.go Outdated Show resolved Hide resolved
translator/trace/jaeger/protospan_to_jaegerproto.go Outdated Show resolved Hide resolved
@sjkaris
Copy link

sjkaris commented Feb 27, 2019

I think unfortunately you will have a not-so-clean merge on rebasing with master again, but everything pre rebase LGTM. Once you rebase I can go over quickly one last time :) Thanks for the contribution!

@annanay25 annanay25 force-pushed the jaeger-grpc-exporter branch from 2e02e83 to 386acaa Compare February 27, 2019 19:20
@annanay25
Copy link
Contributor Author

@sjkaris I've rebased and pushed changes, I hope these were the only changes required. Will check again after you review :)

@flands flands modified the milestones: 0.1.3, 0.1.4 Feb 27, 2019
Copy link

@sjkaris sjkaris left a comment

Choose a reason for hiding this comment

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

Just a couple more nits but otherwise LGTM. @pjanotti will need to give final sign-off though.

@annanay25 annanay25 force-pushed the jaeger-grpc-exporter branch 2 times, most recently from 6b24c41 to 9454050 Compare March 5, 2019 19:38
@songy23 songy23 mentioned this pull request Mar 13, 2019
@annanay25 annanay25 force-pushed the jaeger-grpc-exporter branch from d68f00f to b56d0ef Compare March 14, 2019 05:00
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@songy23 songy23 merged commit b597ff0 into census-instrumentation:master Mar 14, 2019
// NewJaegerProtoGRPCSender returns a new GRPC-backend span sender.
// The collector endpoint should be of the form "hostname:14250".
func NewJaegerProtoGRPCSender(collectorEndpoint string, zlogger *zap.Logger) *JaegerProtoGRPCSender {
client, err := grpc.Dial(collectorEndpoint, grpc.WithInsecure())
Copy link

Choose a reason for hiding this comment

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

FYI jaeger-collector supports TLS connections now: jaegertracing/jaeger#1311

// The collector endpoint should be of the form "hostname:14250".
func NewJaegerProtoGRPCSender(collectorEndpoint string, zlogger *zap.Logger) *JaegerProtoGRPCSender {
client, err := grpc.Dial(collectorEndpoint, grpc.WithInsecure())
zlogger.Fatal("Failed to dail grpc connection", zap.Error(err))
Copy link

Choose a reason for hiding this comment

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

typo: s/dail/dial/

@annanay25
Copy link
Contributor Author

Thanks for the reviews @benley!

@benley
Copy link

benley commented Mar 14, 2019

Thanks for implementing this feature, @annanay25!

fivesheep pushed a commit to fivesheep/opencensus-service that referenced this pull request Jun 12, 2019
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.

7 participants