-
Notifications
You must be signed in to change notification settings - Fork 839
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
Deprecate InstrumentationLibraryInfo #4256
Deprecate InstrumentationLibraryInfo #4256
Conversation
byte[] schemaUrlUtf8, | ||
List<Marshaler> logMarshalers) { | ||
int size = 0; | ||
size += | ||
MarshalerUtil.sizeMessage( | ||
InstrumentationLibraryLogs.INSTRUMENTATION_LIBRARY, instrumentationLibrary); | ||
InstrumentationLibraryLogs.INSTRUMENTATION_LIBRARY, instrumentationScope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just merged the proto update, so think you need to merge main and update the keys now when marshaling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks like the proto PR to change the field names hasn't merged yet: open-telemetry/opentelemetry-proto#362
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh sorry for assuming without actually checking
@@ -10,18 +10,21 @@ | |||
|
|||
import org.junit.jupiter.api.Test; | |||
|
|||
/** Tests for {@link InstrumentationLibraryInfo}. */ | |||
/** Tests for {@link io.opentelemetry.sdk.common.InstrumentationLibraryInfo}. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some qualified imports got automatically added by intellij or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to qualify the imports, even in comments, or else you get an error for using deprecated classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thought if it's in the same package that wouldn't be an issue but maybe not
…y-java into deprecate-instrumentation-library
Codecov Report
@@ Coverage Diff @@
## main #4256 +/- ##
============================================
- Coverage 90.32% 90.19% -0.14%
- Complexity 4752 4753 +1
============================================
Files 553 554 +1
Lines 14611 14629 +18
Branches 1402 1403 +1
============================================
- Hits 13197 13194 -3
- Misses 954 975 +21
Partials 460 460
Continue to review full report at Codecov.
|
…y-java into deprecate-instrumentation-library
We all missed that this was a breaking change, since we added methods to public interfaces without a default implementation. :( |
Related to #4190.