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.

Sign up for GitHub

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

Jump to bottom

Move exemplar types to non-internal package #5747

Merged
merged 14 commits into from
Sep 26, 2024
Merged

Move exemplar types to non-internal package #5747

merged 14 commits into from
Sep 26, 2024

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Aug 27, 2024 •
edited
Loading

Part of #5249

This makes all existing types designed to implement the public Exemplar API public by moving most of internal/exemplar to exemplar. The only types that are not being made public are exemplar.Drop, and exemplar.FilteredReservoir. Those types are moved to internal/aggregate, and are renamed to DropReservoir and FilteredExemplarReservoir.

The following types are made public:

  • exemplar.Exemplar
  • exemplar.Filter
  • exemplar.SampledFilter
  • exemplar.AlwaysOnFilter
  • exemplar.HistogramReservoir
  • exemplar.FixedSizeReservoir
  • exemplar.Reservoir
  • exemplar.Value
  • exemplar.ValueType

@codecov Codecov
Copy link

codecov bot commented Aug 27, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (6edc7a6) to head (204b29d).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5747   +/-   ##
=====================================
  Coverage   84.5%   84.5%           
=====================================
  Files        272     272           
  Lines      22734   22734           
=====================================
+ Hits       19226   19228    +2     
+ Misses      3165    3163    -2     
  Partials     343     343

see 21 files with indirect coverage changes

@dashpole
Copy link
Contributor Author

dashpole commented Sep 4, 2024

The README link doesn't work yet because the package isn't released. I assume I should just leave it there anyways?

@dashpole
Copy link
Contributor Author

@MrAlias rebased

XSAM
XSAM approved these changes Sep 19, 2024
View reviewed changes
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

👍

@MrAlias
Copy link
Contributor

MrAlias commented Sep 19, 2024 •
edited
Loading

Should we rename FixedSize and Histogram to FixedSizeReservoir and HistogramReservoir? Or is it enough, based on the function signature, that a user can figure out what it returns?

@XSAM
Copy link
Member

XSAM commented Sep 19, 2024

Should we rename FixedSize and Histogram to FixedSizeReservoir and HistogramReservoir?

I vote for this.

@dashpole
Copy link
Contributor Author

Renamed. I also took the liberty to rename the files to fixed_size_reservoir.go and histogram_reservoir.go while we are renaming things...

@dashpole
Copy link
Contributor Author

Possibly a follow-up question, but should we name these NewFixedSizeReservoir and NewHistogramReservoir, and export concrete types FixedSizeReservoir and HistogramReservoir? I can't think of why we would need methods on those other than implementing the Reservoir interface, but it is a generally good practice to accept interfaces and return concrete types.

@dashpole dashpole requested a review from XSAM September 20, 2024 15:48
@XSAM
Copy link
Member

XSAM commented Sep 20, 2024

should we name these NewFixedSizeReservoir and NewHistogramReservoir

Looks good.

export concrete types FixedSizeReservoir and HistogramReservoir

I don't have opinions on this.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 20, 2024

Possibly a follow-up question, but should we name these NewFixedSizeReservoir and NewHistogramReservoir, and export concrete types FixedSizeReservoir and HistogramReservoir? I can't think of why we would need methods on those other than implementing the Reservoir interface, but it is a generally good practice to accept interfaces and return concrete types.

Both of these ideas sounds good to me, 👍

@dashpole dashpole force-pushed the public_exemplar branch 3 times, most recently from 39aa6f7 to 706e4ff Compare September 21, 2024 17:41
@dashpole
Copy link
Contributor Author

@MrAlias i'm waiting for you review before merging. Let me know if I should merge without your approval.

MrAlias
MrAlias reviewed Sep 26, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
sdk/metric/exemplar/value.go Outdated Show resolved Hide resolved
sdk/metric/exemplar/fixed_size_reservoir.go Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit 481f498 into open-telemetry:main Sep 26, 2024
31 of 32 checks passed
@MrAlias
Copy link
Contributor

MrAlias commented Sep 26, 2024

Planned follow up discussed in SIG meeting: Audit the compliance of what was just merged with the specification.

TODO: track ^ in an issue.

@dashpole dashpole deleted the public_exemplar branch September 26, 2024 20:26
@pellared
Copy link
Member

LGTM. Should we add some testable example showcasing exemplar-reservoir feature?

@MrAlias MrAlias added this to the v1.31.0 milestone Oct 10, 2024
@dashpole dashpole mentioned this pull request Oct 11, 2024
Merged
dashpole added a commit that referenced this pull request Oct 11, 2024
@dashpole
Release v1.31.0/v0.53.0/v0.7.0/v0.0.10 (#5883)
bc2fe88 
### Added

- Add `go.opentelemetry.io/otel/sdk/metric/exemplar` package which
includes `Exemplar`, `Filter`, `TraceBasedFilter`, `AlwaysOnFilter`,
`HistogramReservoir`, `FixedSizeReservoir`, `Reservoir`, `Value` and
`ValueType` types. These will be used for configuring the exemplar
reservoir for the metrics sdk. (#5747, #5862)
- Add `WithExportBufferSize` option to log batch processor.(#5877)

### Changed

- Enable exemplars by default in `go.opentelemetry.io/otel/sdk/metric`.
Exemplars can be disabled by setting
`OTEL_METRICS_EXEMPLAR_FILTER=always_off` (#5778)
- `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly
introduced `EnabledParameters` type instead of `Record`. (#5791)
- `FilterProcessor.Enabled` in
`go.opentelemetry.io/otel/sdk/log/internal/x` now accepts
`EnabledParameters` instead of `Record`. (#5791)
- The `Record` type in `go.opentelemetry.io/otel/log` is no longer
comparable. (#5847)
- Performance improvements for the trace SDK `SetAttributes` method in
`Span`. (#5864)
- Reduce memory allocations for the `Event` and `Link` lists in `Span`.
(#5858)
- Performance improvements for the trace SDK `AddEvent`, `AddLink`,
`RecordError` and `End` methods in `Span`. (#5874)

### Deprecated

- Deprecate all examples under `go.opentelemetry.io/otel/example` as
they are moved to [Contrib
repository](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/examples).
(#5854)

### Fixed

- The race condition for multiple `FixedSize` exemplar reservoirs
identified in #5814 is resolved. (#5819)
- Fix log records duplication in case of heterogeneous resource
attributes by correctly mapping each log record to it's resource and
scope. (#5803)
- Fix timer channel drain to avoid hanging on Go 1.23. (#5868)
- Fix delegation for global meter providers, and panic when calling
otel.SetMeterProvider. (#5827)
- Change the `reflect.TypeOf` to use a nil pointer to not allocate on
the heap unless necessary. (#5827)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XSAM XSAM XSAM approved these changes

@MrAlias MrAlias MrAlias approved these changes

@dmathieu dmathieu dmathieu approved these changes

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@pellared pellared Awaiting requested review from pellared pellared is a code owner

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.31.0
Development

Successfully merging this pull request may close these issues.
5 participants
@dashpole @MrAlias @XSAM @pellared @dmathieu