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

deps: bumps to Brave 6.0.0 and moves off internal type #511

Merged

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Jan 7, 2024

This updates to Brave 6.0.0 and dodges an internal type that is not in Brave 6.0.

@@ -16,7 +16,6 @@
package io.micrometer.tracing.brave.bridge;

import brave.internal.baggage.BaggageFields;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is also an internal type, but not being removed so far.. maybe by accident ;)

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that was the reason, thanks for mentioning this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think I copied this code from OTel and adopted it to Brave. It was a long time ago so I don't really remember 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could have been. in any case this variant was done a while back benchmarked etc to be same perf as the built-in b3 stuff. hopefully it helps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah definitely was otel, not just your comments say that, but the impl is wrong!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's a PR to re-use the code I wrote a few years ago, highlighting the uberbug in the current impl #532

@codefromthecrypt codefromthecrypt marked this pull request as draft January 8, 2024 11:59
@codefromthecrypt
Copy link
Contributor Author

hold on this one until brave 6 is out as I want to cross-check it

@codefromthecrypt codefromthecrypt changed the title deps: bumps to Brave 5.17.1 and moves off internal type deps: bumps to Brave 6.0.0 and moves off internal type Jan 9, 2024
@codefromthecrypt codefromthecrypt force-pushed the brave-5.17.1 branch 2 times, most recently from de4bb2c to e825600 Compare January 9, 2024 05:05
@codefromthecrypt codefromthecrypt marked this pull request as ready for review January 9, 2024 05:05
@codefromthecrypt codefromthecrypt force-pushed the brave-5.17.1 branch 2 times, most recently from e672867 to e68bede Compare January 9, 2024 07:19
@@ -20,7 +21,12 @@ awaitility = "4.2.0"
mockito = "5.8.0"
wiremock = "3.0.1"
testcontainers = "1.19.3"
braveBom = "5.17.0"
braveBom = "6.0.0"
# TODO: Update to 3.1.0 after https://github.com/open-telemetry/opentelemetry-java/pull/6129
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hope these notes make sense cc @marcingrzejszczak!

This updates to Brave 6.0.0 and dodges an internal type that is not
in Brave 6.0.

Signed-off-by: Adrian Cole <[email protected]>
@marcingrzejszczak marcingrzejszczak merged commit 5bbc418 into micrometer-metrics:main Jan 11, 2024
1 of 5 checks passed
@marcingrzejszczak marcingrzejszczak added enhancement New feature or request dependency-upgrade A dependency upgrade labels Jan 11, 2024
@codefromthecrypt codefromthecrypt deleted the brave-5.17.1 branch January 11, 2024 22:46
@jonatan-ivanov jonatan-ivanov added this to the 1.3.0-M1 milestone Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency-upgrade A dependency upgrade enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants