这是indexloc提供的服务,不要输入任何密码
Skip to content

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 January 20, 2024 00:34
@codecov
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. 🙃

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.

2 participants