Add Marshalers for profiling signal type#6565
Merged
jack-berg merged 6 commits intoopen-telemetry:mainfrom Jul 23, 2024
Merged
Conversation
Utility functions to support required data types.
Marshaler classes for Profile and its constituent types.
Unit tests for profile marshaling.
Codecov ReportAttention: Patch coverage is
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. |
jack-berg
reviewed
Jul 19, 2024
Member
jack-berg
left a comment
There was a problem hiding this comment.
Just a couple of minor comments, but looks pretty good!
exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/Serializer.java
Outdated
Show resolved
Hide resolved
...s/otlp/profiles/src/main/java/io/opentelemetry/exporter/otlp/profiles/FunctionMarshaler.java
Outdated
Show resolved
Hide resolved
...rters/otlp/profiles/src/main/java/io/opentelemetry/exporter/otlp/profiles/LineMarshaler.java
Outdated
Show resolved
Hide resolved
Changes addressing review comments.
Changes addressing review comments.
Move proto wire generation, per review comments.
jack-berg
approved these changes
Jul 23, 2024
breedx-splk
pushed a commit
to breedx-splk/opentelemetry-java
that referenced
this pull request
Aug 12, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.