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

Add Marshalers for profiling signal type #6565

Merged
merged 6 commits into from
Jul 23, 2024

Conversation

jhalliday
Copy link
Contributor

The third step towards support for the new experimental profiling OTLP signal type, following on from the data model interfaces and implementation.

These marshaler classes are somewhat verbose and very boilerplatey, but should not contain any surprises, being patterned largely on the existing marshalers for other signal types. The code is tedious to review, particularly to check the field names and data types correspond correctly to the proto file. I think I got all the cut and paste errors out, but then again I thought that on two previous occasions whilst checking it and proved myself wrong both times. Should probably have written a codegen tool instead, but hey, hindsight...

The classes here cover the payload, pprofextended.proto The PR is already a bit big, so the ones for the envelope, profiles.proto, will follow later.

The utility classes in common are extended with some additional methods to support data types used by profiling but not other signal types.

For the first time we actually get some test coverage on profiling code. The tests for the marshalers use the data model interface and impl, so those get exercised too. May even have gone a bit too far - the code and tests were written starting from the leaf nodes, whereas just testing serialization of the root type would have transitively tested the rest. I'm leaving all the tests in for now, but they could be curated a little to reduce duplication if desired.

Utility functions to support required data types.
Marshaler classes for Profile and its constituent types.
Unit tests for profile marshaling.
@jhalliday jhalliday requested a review from a team July 9, 2024 13:18
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 0% with 56 lines in your changes missing coverage. Please review.

Project coverage is 90.31%. Comparing base (09c7c4a) to head (2067223).
Report is 19 commits behind head on main.

Files Patch % Lines
...elemetry/exporter/internal/marshal/Serializer.java 0.00% 28 Missing ⚠️
...metry/exporter/internal/marshal/MarshalerUtil.java 0.00% 23 Missing ⚠️
...try/exporter/internal/marshal/ProtoSerializer.java 0.00% 3 Missing ⚠️
...etry/exporter/internal/marshal/JsonSerializer.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6565      +/-   ##
============================================
- Coverage     90.59%   90.31%   -0.29%     
- Complexity     6255     6261       +6     
============================================
  Files           689      690       +1     
  Lines         18698    18762      +64     
  Branches       1843     1853      +10     
============================================
+ Hits          16940    16945       +5     
- Misses         1201     1258      +57     
- Partials        557      559       +2     

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

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments, but looks pretty good!

Changes addressing review comments.
Changes addressing review comments.
Move proto wire generation, per review comments.
@jack-berg jack-berg merged commit 26b3d41 into open-telemetry:main Jul 23, 2024
16 of 18 checks passed
breedx-splk pushed a commit to breedx-splk/opentelemetry-java that referenced this pull request Aug 12, 2024
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