Skip to content

Conversation

@making
Copy link
Member

@making making commented Aug 7, 2019

@shakuzen
Copy link
Member

shakuzen commented Aug 7, 2019

We might want to wait on merging this for the outcome of the discussion on openzipkin vs zipkin in the logo.

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Aug 7, 2019 via email

@making
Copy link
Member Author

making commented Aug 8, 2019

Updated

image

@codefromthecrypt
Copy link
Member

ahead of the curve huh? :D Screenshot 2019-08-08 at 1 21 31 PM

@making
Copy link
Member Author

making commented Aug 8, 2019

I tested banner.txt in a latest spring boot app.

@codefromthecrypt
Copy link
Member

if you can verify this is correct still with the new logo content: openzipkin/openzipkin.github.io#142 then later or now we still need to update lens images

@codefromthecrypt
Copy link
Member

only difference is if we think red or orange, I guess

Copy link
Member

@jeqo jeqo left a comment

Choose a reason for hiding this comment

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

sadly, seems that orange is not available https://docs.spring.io/spring-boot/docs/current/api/org/springframework/boot/ansi/AnsiColor.html but I will let @making confirm if possible.
we might fallback to white as well.

@making
Copy link
Member Author

making commented Sep 17, 2019

@jeqo @adriancole as far as I know, orange is not supposed.

@codefromthecrypt
Copy link
Member

I think black/white/grey is better than red when orange unsupported. Red is a more aggressive color.

can you raise an issue to support orange :)

@making
Copy link
Member Author

making commented Sep 17, 2019

I'll try to implement a new org.springframework.boot.ansi.AnsiElement so that it supports 256 colors.

Which color do you prefer? :)
image

You can get this table by

for i in $(seq 1 255);do printf "%03d \033[38;5;${i}mZipkin\033[0;00m\t" $i; if [ $(( $i % 10 )) -eq 0 ];then echo;fi ;done;echo

@making
Copy link
Member Author

making commented Sep 17, 2019

I think I managed to change the color :)

image

Let me update the source code later

@jorgheymans
Copy link
Contributor

I think I managed to change the color :)

image

Let me update the source code later

Cool ! Will it work in a windows shell as well ?

@making
Copy link
Member Author

making commented Sep 17, 2019

I've just pushed the updated banner in 4ca0ae3.

image

@jorgheymans I don't have a Windows environment. Could you please give it a try?

@making
Copy link
Member Author

making commented Sep 17, 2019

In 4ca0ae3
I added ZipkinAnsi256Color, ZipkinAnsi256PropertySource and ZipkinBanner, then configured the banner programmatically as described in the boot doc in order to support 256 colors that is not available in Spring Boot out of the box now.
I'm thinking to contribute to Spring Boot but it wouldn't come to Spring Boot 2.1.x. So Zipkin needs to have these classes until a feature version.


import org.springframework.boot.ansi.AnsiElement;

public class ZipkinAnsi256Color implements AnsiElement {
Copy link
Member

Choose a reason for hiding this comment

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

add a TODO to delete on spring boot 2.2 if that's when this is scheduled to be normally possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in edebc31

@codefromthecrypt
Copy link
Member

Will merge this on green.

Added issue to update lens #2806

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.

thanks again!

@codefromthecrypt codefromthecrypt merged commit f47e930 into openzipkin:master Sep 18, 2019
@making
Copy link
Member Author

making commented Sep 22, 2019

It's been merged in Spring Boot 2.2 :)
spring-projects/spring-boot#18264

@making making deleted the patch-1 branch September 22, 2019 07:41
@codefromthecrypt
Copy link
Member

codefromthecrypt commented Sep 22, 2019 via email

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.

6 participants