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

feat(otlp-transformer)!: accept ResourceMetrics instead of ResoruceMetrics[] in metrics serializers #5159

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Nov 14, 2024

Which problem is this PR solving?

See #3276 - the old exporter bases did not support non-array export types so we worked around it by having a proxy exporter class that took ResourceMetrics[] that does not match the PushMetricExporter interface (which requires ResourceMetrics), and a base implementation that matches the PushMetricExporter interface and converts the ResourceMetrics into ResourceMetrics[]. As the issue linked above states, this was not optimal and looked quite confusing to most.

However, with recent changes in #5031, #4542, #4971, #4895 and #4743, we don't need this workaround anymore and can now use a more direct approach.

This PR accomplishes this by aligining serializer to take ResourceMetricsas provided by the PushMetricExporter interface instead of ResourceMetrics[] and delegates wrapping ResourceMetrics to the ISerializer implementation.

Fixes #3276

Breaking changes

  • (user-facing): ProtobufMetricsSerializer now only accepts ResourceMetrics instead of ResourceMetrics[] to align with PushMetricExporter requirements
  • (user-facing): JsonMetricsSerializer now only accepts ResourceMetrics instead of ResourceMetrics[] to align with PushMetricExporter requirements

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Existing tests

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.55%. Comparing base (2a5e0d8) to head (adf81b1).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5159      +/-   ##
==========================================
- Coverage   94.57%   94.55%   -0.02%     
==========================================
  Files         314      314              
  Lines        7961     7956       -5     
  Branches     1600     1600              
==========================================
- Hits         7529     7523       -6     
- Misses        432      433       +1     
Files with missing lines Coverage Δ
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 89.06% <100.00%> (-0.80%) ⬇️
.../packages/otlp-transformer/src/json/serializers.ts 100.00% <100.00%> (ø)
...kages/otlp-transformer/src/protobuf/serializers.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

@pichlermarc pichlermarc force-pushed the feat/simplify-metrics-exporter-base branch from f78b85d to 1e5895b Compare November 19, 2024 14:07
@pichlermarc pichlermarc marked this pull request as ready for review November 19, 2024 14:15
@pichlermarc pichlermarc requested a review from a team as a code owner November 19, 2024 14:15
@pichlermarc pichlermarc added this pull request to the merge queue Nov 20, 2024
Merged via the queue into open-telemetry:main with commit 91b9abd Nov 20, 2024
22 checks passed
@pichlermarc pichlermarc deleted the feat/simplify-metrics-exporter-base branch November 20, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor OTLP Metrics Exporters
2 participants