Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

deps: bumps to Brave 5.18.1 and moves off internal type #2335

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

Closed

Conversation

@codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Jan 7, 2024
edited
Loading

This updates to Brave 5.18.0 and dodges an internal type that will not be in Brave 6.0. See openzipkin/brave#1396 about Propagation and Brave 6.0

Copy link

@codefromthecrypt Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Copy link

this.braveBaggageManager = braveBaggageManager;
}

/**
Copy link
Contributor Author

@codefromthecrypt codefromthecrypt Jan 7, 2024

Choose a reason for hiding this comment

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

p.s I'm not sure if this was copied because brave's wasn't published to maven central. It is now, so maybe can reduce maintenance here https://central.sonatype.com/artifact/io.zipkin.contrib.brave-propagation-w3c/brave-propagation-tracecontext/versions from https://github.com/openzipkin-contrib/brave-propagation-w3c

@codefromthecrypt codefromthecrypt changed the title (削除) deps: bumps to Brave 5.17.1 and moves off internal type (削除ここまで) (追記) deps: bumps to Brave 5.18/0 and moves off internal type (追記ここまで) Jan 8, 2024
@codefromthecrypt codefromthecrypt changed the title (削除) deps: bumps to Brave 5.18/0 and moves off internal type (削除ここまで) (追記) deps: bumps to Brave 5.18.0 and moves off internal type (追記ここまで) Jan 8, 2024
<artifactId>zipkin</artifactId>
<optional>true</optional>
</dependency>
<!-- Needed for ZipkinRestTemplateSenderConfiguration: BytesMessageEncoder -->
Copy link
Contributor Author

@codefromthecrypt codefromthecrypt Jan 8, 2024

Choose a reason for hiding this comment

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

maybe this code should be rewritten to avoid classes or this dep become not optional?

backend | 2024年01月08日 05:03:14.185 [] [/] WARN [main] o.s.c.s.AbstractApplicationContext - Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'zipkinSender' defined in class path resource [org/springframework/cloud/sleuth/zipkin2/sender/ZipkinRestTemplateSenderConfiguration.class]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [zipkin2.reporter.Sender]: Factory method 'restTemplateSender' threw exception; nested exception is java.lang.NoSuchMethodError: 'zipkin2.reporter.BytesMessageEncoder zipkin2.reporter.BytesMessageEncoder.forEncoding(zipkin2.codec.Encoding)'

Copy link
Contributor Author

@shakuzen @marcingrzejszczak @ryanjbaxter I got this as far as I can.. there's an unrelated error below, which seems likely due to the runner using too new a JDK

org.springframework.cloud.sleuth.instrument.messaging.BraveTraceFunctionAwareWrapperTests.test_tracing_with_function() Time elapsed: 1 sec <<< FAILURE!
java.lang.ClassCastException: class java.lang.String cannot be cast to class org.springframework.messaging.Message (java.lang.String is in module java.base of loader 'bootstrap'; org.springframework.messaging.Message is in unnamed module of loader 'app')

The summary is that this change allows an upgrade to brave 5.18 or 6.0, but while sleuth supplies and consumes AsyncReporter types, it cannot upgrade to zipkin-reporter 2. This may be fine at least for a while, as Brave 6 doesn't care about zipkin or zipkin-reporter versions anymore.

In a bit, I will update this with Brave 6 (need to release that, first)

Copy link
Member

jonatan-ivanov commented Jan 8, 2024
edited
Loading

Hi Adrian, thanks for the PR! :)

Sleuth is already over its OSS support EOL date so Brave 6 might not be a concern.
I think a minor version upgrade is a good question: we are not planning to release a new minor version of Sleuth only patch versions for commercial users.

On the other hand, we should do this in Micrometer Tracing as it is the successor of Sleuth.

Copy link
Contributor Author

ok so what I'll do is cross-test this PR with Brave 5.18 and 6 and then close it if it won't be merged. Then, in case you need the code later, you know where to look!

jonatan-ivanov reacted with thumbs up emoji

@codefromthecrypt codefromthecrypt changed the title (削除) deps: bumps to Brave 5.18.0 and moves off internal type (削除ここまで) (追記) deps: bumps to Brave 5.18.1 and moves off internal type (追記ここまで) Jan 9, 2024
Copy link
Contributor Author

So, the main thing is that if this PR isn't merged and released, users can't upgrade to Brave 5.18 as things that wrap Propagation.Factory don't wrap .get(). So, they will be stuck at 5.17.1 regardless of zipkin-reporter or deprecation.

If it is merged, likely this will work with Brave 6 as well, just the messaging tests are broken and until they aren't I'm not sure if there is another issue.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Copy link
Member

I think that might be a compelling argument for merging this even though we are not planning to do another minor release. We can also keep it open to see if there is user interest.

Copy link
Contributor

I'm looking into this and I'm getting some NoClassDefFoundError. I'll look into that

itialization - cancelling refresh attempt: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'kafkaStreamsTracing' defined in class path resource [org/springframework/cloud/sleuth/autoconfig/brave/instrument/messaging/BraveKafkaStreamsAutoConfiguration.class]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [brave.kafka.streams.KafkaStreamsTracing]: Factory method 'kafkaStreamsTracing' threw exception; nested exception is java.lang.NoClassDefFoundError: org/apache/kafka/streams/processor/api/FixedKeyProcessorSupplier

Copy link
Contributor Author

glad to hear you are looking at the build. I think people will be using boot 2 for many years.. if we can get a bit more life extension to sleuth to the community, I'm sure they will appreciate it!

Copy link
Contributor

Done via 8f39760, polished via 8315592

Copy link
Contributor Author

this works provably. While people can't upgrade zipkin-reporter to a new version, they can upgrade brave without a snag: openzipkin/brave-example#113

For most people, since they are using sleuth's reporters anyway, there's little problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

3.1.11

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /