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

Flatten sdk/metric/aggregation into sdk/metric #4435

Merged
merged 20 commits into from
Aug 14, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 10, 2023

Resolve #4432

  • Move the Aggregation interface and its implementations to sdk/metric
    • Unexport all methods of the Aggregation interface
    • Prepend Aggregation to all concrete implementations of the Aggregation interface
  • Deprecate the sdk/metric/aggregation pkg
  • Decouple sdk/metric/internal/aggregate from the aggregation types so there is no import cycle
  • Stop using the previously exported methods of Aggregator in the metric exporters
    • There is no real need validate the aggregation selector these exporters receive
  • Update all use of the deprecated sdk/metric/aggregation with sdk/metric

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Aug 10, 2023
@MrAlias MrAlias added this to the v1.17.0/v0.40.0 milestone Aug 10, 2023
@MrAlias MrAlias force-pushed the dep-aggregation-pkg branch from b25409b to 44d86ea Compare August 10, 2023 21:01
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #4435 (424c234) into main (199dc34) will increase coverage by 0.0%.
Report is 1 commits behind head on main.
The diff coverage is 79.2%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4435   +/-   ##
=====================================
  Coverage   78.8%   78.9%           
=====================================
  Files        253     254    +1     
  Lines      20668   20650   -18     
=====================================
- Hits       16307   16305    -2     
+ Misses      4012    4000   -12     
+ Partials     349     345    -4     
Files Changed Coverage Δ
exporters/otlp/otlpmetric/internal/exporter.go 67.2% <0.0%> (ø)
sdk/metric/aggregation/aggregation.go 69.0% <ø> (-12.0%) ⬇️
sdk/metric/instrument.go 92.0% <ø> (ø)
sdk/metric/pipeline.go 85.9% <51.6%> (-2.6%) ⬇️
sdk/metric/aggregation.go 88.8% <88.8%> (ø)
...xporters/otlp/otlpmetric/internal/oconf/options.go 83.5% <100.0%> (+3.6%) ⬆️
...porters/otlp/otlpmetric/otlpmetricgrpc/exporter.go 83.0% <100.0%> (ø)
...pmetric/otlpmetricgrpc/internal/oconf/envconfig.go 93.1% <100.0%> (ø)
...tlpmetric/otlpmetricgrpc/internal/oconf/options.go 87.8% <100.0%> (+3.7%) ⬆️
...porters/otlp/otlpmetric/otlpmetrichttp/exporter.go 83.0% <100.0%> (ø)
... and 11 more

... and 1 file with indirect coverage changes

@MrAlias MrAlias marked this pull request as ready for review August 10, 2023 21:09
CHANGELOG.md Outdated Show resolved Hide resolved
sdk/metric/aggregation.go Outdated Show resolved Hide resolved
@pellared
Copy link
Member

I approved as I am going on PTO and the comments to be addressed are trivial.

@dashpole
Copy link
Contributor

I like the move overall. Sum -> AggregationSum needs to be fixed

Co-authored-by: Robert Pająk <[email protected]>
@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 11, 2023

Package site index:

20230811_080600

This naming strategy accomplishes the intended grouping of aggregations in docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Move aggregation to sdk/metric
3 participants