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

custom servers are not supported ! #2791

Merged
merged 1 commit into from Sep 12, 2019

Conversation

@jorgheymans
Copy link
Contributor

commented Aug 30, 2019

No description provided.

@@ -73,8 +73,7 @@ Note: The above is just an example, most likely you'll want to use an existing t
## Storage Component
Zipkin includes a [StorageComponent](zipkin/src/main/java/zipkin2/storage/StorageComponent.java), used to store and query spans and
dependency links. This is used by the server and those making custom
servers, collectors, or span reporters. For this reason, storage
dependency links. This is used by the server and those making collectors, or span reporters. For this reason, storage

This comment has been minimized.

Copy link
@adriancole

adriancole Aug 31, 2019

Contributor

"span reporters" is quite the edge case for the storage component. in hind sight I think we should be more cautious about this in general, perhaps remove it from the top-level readme.

I'm thinking move section about storage component to readme under zipkin-storage, saying the only use case is server-development and while we keep dependencies down etc, the storage and collector apis primarily are in service to server extensions and are not intended for people to nest into traced applications

This comment has been minimized.

Copy link
@shakuzen

shakuzen Sep 2, 2019

Member

Adrian's suggestion here sounds good to me. Better to get this out of the top-level README so people aren't tempted to try this kind of thing.

@jorgheymans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@adriancole

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

you have time to land this? @fbeaufume had some comments that make this such a relevant pull request. I suspect even after we land this folks will still do custom servers and not look at our docs that ask please do not.

https://github.com/openzipkin/zipkin/blob/master/zipkin-server/src/main/java/zipkin/server/EnableZipkinServer.java#L25-L33

@adriancole

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

I think maybe we need to go from clear to aggressively clear

for example

"That zipkin uses spring boot at all is an implementation detail that can change at any time. We do not support custom servers outside our org. Anything else is completely use at your own risk"

cc @openzipkin/core

@basvanbeek

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Suggesting minor changes...

"That Zipkin uses spring boot at all is solely an internal implementation detail that can change at any time. We do NOT support custom servers outside our org! Anything else is completely use at your own risk!!!"

@jeqo

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

I'd also suggest to add a header with anchor to link directly to.

@jorgheymans

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@adriancole you can go ahead and apply this already. Rest of the doc patches will follow the next couple of days.

@adriancole adriancole merged commit 40fa08d into openzipkin:master Sep 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adriancole

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

thanks @jorgheymans

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.