Skip to content

Conversation

@kotharironak
Copy link
Contributor

@kotharironak kotharironak commented Jul 13, 2021

Description

This is in the context of

Testing

  • Fixed the migration test after the change.

Note

  • all the dependent (consumer) changes to make this possible have been merged.
  • we will also be removing these fields from the data model as a follow-up.

Copy link
Contributor

@findingrish findingrish left a comment

Choose a reason for hiding this comment

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

Build is failing, tests have to be updated accordingly

@kotharironak
Copy link
Contributor Author

Build is failing, tests have to be updated accordingly

But, do we need this test now? It's trying to covert to RawSpna. I can use a field generator.

@kotharironak kotharironak marked this pull request as ready for review July 19, 2021 10:12
@kotharironak kotharironak requested a review from a team July 19, 2021 10:12
@codecov
Copy link

codecov bot commented Jul 19, 2021

Codecov Report

Merging #222 (fa7608b) into main (3b7df2b) will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #222      +/-   ##
============================================
- Coverage     80.69%   80.50%   -0.20%     
+ Complexity     1129     1126       -3     
============================================
  Files           101      101              
  Lines          4352     4349       -3     
  Branches        400      400              
============================================
- Hits           3512     3501      -11     
- Misses          654      659       +5     
- Partials        186      189       +3     
Flag Coverage Δ
unit 80.50% <ø> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...re/spannormalizer/jaeger/JaegerSpanNormalizer.java 75.51% <ø> (-0.73%) ⬇️
...pannormalizer/fieldgenerators/FieldsGenerator.java 78.57% <0.00%> (-11.91%) ⬇️
...normalizer/fieldgenerators/RpcFieldsGenerator.java 91.07% <0.00%> (-5.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b7df2b...fa7608b. Read the comment docs.

@github-actions

This comment has been minimized.

rawSpan.getEvent().getHttp().getRequest().getUserAgent(),
HttpSemanticConventionUtils.getHttpUserAgent(rawSpan.getEvent()).get());
// now, we are not populating first class fields. So, it should be null.
assertNull(rawSpan.getEvent().getHttp());
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this repetitive/irrelevant in every test ? Maybe we can have another test just checking nullity of first class fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept it after - RawSpan rawSpan = normalizer.convert("tenant-key", span); - convert call.

@sarthak77
Copy link
Member

We need to merge the changes from main too

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kotharironak kotharironak merged commit 2055b11 into main Jul 20, 2021
@kotharironak kotharironak deleted the disable-first-class-field branch July 20, 2021 05:35
@github-actions
Copy link

Unit Test Results

  69 files  ±0    69 suites  ±0   52s ⏱️ +7s
359 tests ±0  359 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 2055b11. ± Comparison against base commit 3b7df2b.

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.

4 participants