Skip to content
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

Use armeria-grpc client in storage-stackdriver. #120

Merged
merged 7 commits into from
Apr 2, 2019

Conversation

anuraaga
Copy link
Collaborator

One concern is the API change in storage-stackdriver's Builder not sure what the API guarantee on that is.

Only ran integration tests.

Fixes #119

@anuraaga
Copy link
Collaborator Author

Hmm - does the build require updating the license headers to 2019?

@anuraaga
Copy link
Collaborator Author

/cc @openzipkin/armeria

@codefromthecrypt
Copy link
Member

we dont have api guarantees pre 1.0!

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

looks great.

there might be a sneaky dep on grpc as I ran it and got a NCDFE

$ GOOGLE_APPLICATION_CREDENTIALS=/Users/acole/.gcp/credentials.json STORAGE_TYPE=stackdriver STACKDRIVER_PROJECT_ID=zipkin-demo STORAGE_TYPE=stackdriver     java     -Dloader.path='stackdriver.jar,stackdriver.jar!/lib'     -Dspring.profiles.active=stackdriver     -cp zipkin.jar     org.springframework.boot.loader.PropertiesLauncher
--snip--
Caused by: java.lang.NoClassDefFoundError: io/grpc/stub/AbstractStub
	at java.lang.ClassLoader.defineClass1(Native Method) ~[?:1.8.0_181]
	at java.lang.ClassLoader.defineClass(ClassLoader.java:763) ~[?:1.8.0_181]
	at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142) ~[?:1.8.0_181]
	at java.net.URLClassLoader.defineClass(URLClassLoader.java:467) ~[?:1.8.0_181]
	at java.net.URLClassLoader.access$100(URLClassLoader.java:73) ~[?:1.8.0_181]
	at java.net.URLClassLoader$1.run(URLClassLoader.java:368) ~[?:1.8.0_181]
	at java.net.URLClassLoader$1.run(URLClassLoader.java:362) ~[?:1.8.0_181]
	at java.security.AccessController.doPrivileged(Native Method) ~[?:1.8.0_181]
	at java.net.URLClassLoader.findClass(URLClassLoader.java:361) ~[?:1.8.0_181]
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424) ~[?:1.8.0_181]
	at org.springframework.boot.loader.LaunchedURLClassLoader.loadClass(LaunchedURLClassLoader.java:93) ~[zipkin.jar:?]
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357) ~[?:1.8.0_181]
	at zipkin2.storage.stackdriver.StackdriverStorage$Builder.build(StackdriverStorage.java:110) ~[zipkin-storage-stackdriver-0.10.4-SNAPSHOT.jar!/:?]
	at zipkin.autoconfigure.storage.stackdriver.ZipkinStackdriverStorageAutoConfiguration.storage(ZipkinStackdriverStorageAutoConfiguration.java:102) ~[stackdriver.jar!/:?]
	at zipkin.autoconfigure.storage.stackdriver.ZipkinStackdriverStorageAutoConfiguration$$EnhancerBySpringCGLIB$$4cdf73f2.CGLIB$storage$0(<generated>) ~[stackdriver.jar!/:?]
	at zipkin.autoconfigure.storage.stackdriver.ZipkinStackdriverStorageAutoConfiguration$$EnhancerBySpringCGLIB$$4cdf73f2$$FastClassBySpringCGLIB$$71e493cc.invoke(<generated>) ~[stackdriver.jar!/:?]

@codefromthecrypt
Copy link
Member

almost sorted

@codefromthecrypt
Copy link
Member

ok this works now, except we have to bump zipkin's guava as grpc no longer works with guava 19. this requires a release of zipkin before the integration will work via curl downloading etc.

Copy link
Collaborator Author

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Man this is a lot of dependency hell...

sender-stackdriver/pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@codefromthecrypt
Copy link
Member

ok I tested against live stackdriver and all good

@codefromthecrypt codefromthecrypt merged commit c2e44d6 into openzipkin:master Apr 2, 2019
@codefromthecrypt
Copy link
Member

thanks for the hard work @anuraaga!

@codefromthecrypt
Copy link
Member

out at 0.11.0 (into the docker, too)

@trustin
Copy link

trustin commented Apr 3, 2019

Nice work, @anuraaga !

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.

armerify
3 participants