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

Include trace flags in otlp marshaller #6167

Merged
merged 11 commits into from
Jan 31, 2024

Conversation

breedx-splk
Copy link
Contributor

As of version 1.1.0, the OTLP protos now include a flags field, which includes the trace flags.
https://github.com/open-telemetry/opentelemetry-proto/releases/tag/v1.1.0

This change includes these flags in the marshalling of OTLP spans.

@breedx-splk breedx-splk requested a review from a team as a code owner January 20, 2024 00:34
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e4b31ce) 91.00% compared to head (bebee8f) 91.09%.
Report is 10 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6167      +/-   ##
============================================
+ Coverage     91.00%   91.09%   +0.08%     
- Complexity     5646     5676      +30     
============================================
  Files           619      620       +1     
  Lines         16443    16545     +102     
  Branches       1663     1682      +19     
============================================
+ Hits          14964    15071     +107     
+ Misses         1017     1005      -12     
- Partials        462      469       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

class SpanMarshalerTest {

@Test
void marshalToJson() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this test is largely duplicating what is being done in TraceRequestMarshalerTest here: https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/common/src/test/java/io/opentelemetry/exporter/internal/otlp/traces/TraceRequestMarshalerTest.java#L114-L232

Can we just add an additional assertion in that test against the span flags and remove this file?

    assertThat(span.getFlags() & SpanFlags.SPAN_FLAGS_TRACE_FLAGS_MASK_VALUE).isEqualTo(1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, the SpanMarshaler was probably already covered (with tests) through the TraceMarshaller (since it does "real" testing through real collaborators, not mocking). I'll see if I can just get an assertion in there and pull this back. I think I misinterpreted the original ask. 🙃

@jack-berg jack-berg merged commit 1969059 into open-telemetry:main Jan 31, 2024
18 checks passed
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.

None yet

2 participants