-
Notifications
You must be signed in to change notification settings - Fork 843
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
Add asserts for event body fields #6509
Conversation
fb74949
to
4fd6513
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6509 +/- ##
============================================
+ Coverage 89.98% 90.01% +0.03%
- Complexity 6314 6353 +39
============================================
Files 703 703
Lines 19062 19138 +76
Branches 1875 1887 +12
============================================
+ Hits 17152 17228 +76
Misses 1333 1333
Partials 577 577 ☔ View full report in Codecov by Sentry. |
5dedd49
to
3afc32d
Compare
I hid the formerly public method that used AnyValue. Ready for another look. |
sdk/testing/src/test/java/io/opentelemetry/sdk/testing/assertj/LogAssertionsTest.java
Show resolved
Hide resolved
private void bodyIsAnyValue() { | ||
// Actually, we can't do these two checks because the body is still STRING type | ||
// assertThat(actual.getBody().getType()).isNotSameAs(Body.Type.EMPTY); | ||
// assertThat(actual.getBody().getType()).isNotSameAs(Body.Type.STRING); |
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.
I think we can make this assertion now.. See how LogMarshaler checks if body instanceof AnyValueBody
.
I might be misunderstanding though. Definitely need to be careful to not expose AnyValue as part of the public API until its stable.
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.
We're slightly mixing up two things here....but the point was this assert will ALWAYS fail right now, because the body is always still STRING type.
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.
I think its time we stabilized the AnyValue portions of the log API. Opened #6581 to track.
Its strange to have stable APIs asserting the contents of a log record body which are impossible to produce with using an experimental API. As a part of stabilizing we should fix the Body.Type
enum, either by including all the possible value types of AnyValue OR by extending it with a single AnyValue type.
3afc32d
to
3c2ade7
Compare
d8a3a2e
to
6a4be2c
Compare
6a4be2c
to
2a89661
Compare
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 a few nits, but should be able to get it in for Friday's release.
I couldn't find a great way (without casting to
AnyValueBody
) for users to do assertions aroundLogRecordData
bodies, specifically for writing tests involving Events. This seemed like a pretty low-impact way to add some testability.